| Time |
Nick |
Message |
| 00:00 |
Shara |
I don't really see a reason to change obsidian door texture either way |
| 00:01 |
paramat |
hey, user Unarelith (Quent42430 on github) would like to chat here about #7899 and is using Hexchat, but somehow can't talk here, anyone know why? |
| 00:01 |
ShadowBot |
https://github.com/minetest/minetest/issues/7899 -- Architectural improvement needed |
| 00:01 |
paramat |
hm, i think the current handles are awful, but nevermind :) |
| 00:02 |
Shara |
Well, simple straight vertical ones would be nicer. The shape of the suggested change just doesn't map across to so few pixels very well in my opinion |
| 00:03 |
paramat |
ah |
| 00:04 |
paramat |
i might be able to do vertical ones |
| 00:06 |
|
danielp3344 joined #minetest-dev |
| 00:09 |
Unarelith |
. |
| 00:09 |
Unarelith |
ok looks like I'm able to talk here after all |
| 00:10 |
Unarelith |
the most important thing I want to discuss is file splits, for example for content_sao it would look like this: https://github.com/Quent42340/minetest/commit/da9e0c3dc89d857091ebbd89999f69b2429e7e13 |
| 00:12 |
Unarelith |
currently, I can't make a PR because this stuff can break current open PRs since they'll be impossible to merge after mine |
| 00:14 |
Unarelith |
but the goal would be to make new PRs easier to merge (more files = less conflicts) |
| 00:22 |
paramat |
ah good |
| 00:24 |
Unarelith |
I think the main problem is that the code is packed into giant files, so splitting them, sorting them and organizing them should make everything more clear |
| 00:26 |
Unarelith |
but with the side-effect of invalidating old PRs, and this side-effect will always be present since PRs will continue to grow |
| 00:26 |
paramat |
the best devs to talk to about code architecture are nerzhul, sfan5 and rubenwardy, best time is EU daytime, especially evening. i'm not so good with complex code architecture stuff |
| 00:28 |
Unarelith |
ok thanks paramat |
| 00:42 |
|
yusf[m] joined #minetest-dev |
| 00:42 |
|
MayeulC joined #minetest-dev |
| 00:42 |
|
kollaps[m] joined #minetest-dev |
| 00:44 |
|
ANAND joined #minetest-dev |
| 00:46 |
|
rubenwardy joined #minetest-dev |
| 01:29 |
|
dzho_ joined #minetest-dev |
| 01:49 |
|
longerstaff13-m joined #minetest-dev |
| 02:27 |
|
behalebabo joined #minetest-dev |
| 02:31 |
|
reductum joined #minetest-dev |
| 03:15 |
|
Miner_48er joined #minetest-dev |
| 03:20 |
|
indiana joined #minetest-dev |
| 03:50 |
|
Cornelia joined #minetest-dev |
| 04:41 |
|
Cornelia joined #minetest-dev |
| 05:28 |
|
jas__ joined #minetest-dev |
| 05:45 |
|
Cornelia joined #minetest-dev |
| 07:05 |
nerzhul |
Unarelith i agree with you on that point but it's not easy, we started the refactor. This also permits to test the code easily |
| 07:07 |
|
Icedream joined #minetest-dev |
| 07:19 |
|
Icedream joined #minetest-dev |
| 07:22 |
|
Icedream joined #minetest-dev |
| 08:00 |
|
longerstaff13-m joined #minetest-dev |
| 08:14 |
|
Mensious joined #minetest-dev |
| 08:35 |
nerzhul |
i agree about SAO split |
| 08:35 |
nerzhul |
i can provide a such PR |
| 09:02 |
|
troller joined #minetest-dev |
| 09:15 |
|
T^4im joined #minetest-dev |
| 09:17 |
|
ShadowBot` joined #minetest-dev |
| 09:17 |
|
thePalin- joined #minetest-dev |
| 09:20 |
|
_0_ joined #minetest-dev |
| 09:34 |
|
longerstaff13-m joined #minetest-dev |
| 09:36 |
|
longerstaff13-m joined #minetest-dev |
| 09:43 |
p_gimeno |
I don't follow the logic behind "more files = less conflicts". If two changes touch the same area, they are going to conflict no matter the split, and if they touch different areas, they are not going to conflict, and I don't see how separation into files changes that. |
| 10:35 |
|
red-001 joined #minetest-dev |
| 11:40 |
|
proller joined #minetest-dev |
| 11:41 |
nerzhul |
it's not more files, but more objects, somes are too big i think, i don't know how to split all but this can permit to have also more tests |
| 11:41 |
nerzhul |
the shown separation is logical for me but has no real effect |
| 12:41 |
|
Mensious joined #minetest-dev |
| 12:49 |
|
calcul0n joined #minetest-dev |
| 13:18 |
|
Wuzzy joined #minetest-dev |
| 13:44 |
|
calcul0n joined #minetest-dev |
| 13:46 |
Unarelith |
nerzhul, I made the SAO split so I can PR my changes if you want |
| 13:46 |
Unarelith |
actually, I can do this for all the files if needed |
| 13:46 |
nerzhul |
you can, but due to the change i tend to prefer to do it myself to ensure we don't miss any line |
| 13:46 |
nerzhul |
and not it's not java we will never do it on all files :p |
| 13:47 |
Unarelith |
you really should tho :/ |
| 13:48 |
Unarelith |
because "where the fuck is defined PlayerHPChangeReason? at the end of content_sao.h after 400 unrelated lines" is a problem because the answer could be `PlayerHPChangeReason.hpp` |
| 13:50 |
Unarelith |
<nerzhul> you can, but due to the change i tend to prefer to do it myself to ensure we don't miss any line <= and it's mainly copy pasting, I don't how it could be possible to miss lines |
| 13:54 |
nerzhul |
i'm not with you, code review principes |
| 13:56 |
Unarelith |
small commits shouldn't be hard to review actually |
| 14:10 |
Unarelith |
nerzhul, and btw I work in C++ since 8 years so your "no I'll do it because you'll probably miss lines" is really hard to take actually |
| 14:10 |
nerzhul |
Unarelith, it's not against you, error can happen everytime, and don't forget nobody knows you since yesterday |
| 14:13 |
nerzhul |
Unarelith, provide the SAO PR i will use git diff to ensure it's proper |
| 14:13 |
nerzhul |
i'm okay to split this big file |
| 14:13 |
nerzhul |
i think you neverlook server, serverenv or game :p |
| 14:13 |
nerzhul |
i miss time on MT to perform more refactor but we need to refactor many things |
| 14:16 |
Unarelith |
nerzhul, unfortunately my fork had two commits prior to the SAO split: moving every src/ file (which is not already in a folder) into src/engine and src/engine/client. |
| 14:16 |
Unarelith |
and I don't think this kind of change can be easily merged because of existing PRs |
| 14:17 |
nerzhul |
why did you do this, and why did you fork ? |
| 14:17 |
nerzhul |
i agree we need to move our files to more proper folders to have a better project view, it's a long time issue we do |
| 14:17 |
nerzhul |
see 0.4.16 tree to see before :p |
| 14:18 |
Unarelith |
so you have your answer :) |
| 14:18 |
|
Gael-de-Sailly joined #minetest-dev |
| 14:18 |
Unarelith |
my fork was made to give examples for #7899 |
| 14:18 |
ShadowBot |
https://github.com/minetest/minetest/issues/7899 -- Architectural improvement needed |
| 14:20 |
Unarelith |
so I can still make a PR with what I did, but then we'll have to change some older PRs |
| 14:21 |
nerzhul |
not a problem |
| 14:21 |
nerzhul |
open the split the sao PR |
| 14:24 |
Unarelith |
nerzhul, I opened #7900 with what I did on my fork |
| 14:24 |
ShadowBot |
https://github.com/minetest/minetest/issues/7900 -- Global code architecture improvement + SAO split by Quent42340 |
| 14:26 |
|
jas_ joined #minetest-dev |
| 14:28 |
|
DI3HARD139 joined #minetest-dev |
| 15:07 |
Unarelith |
nerzhul, I had to do a full cmake clean to see missing/invalid headers in current build. CMake is supposed to handle that, why isn't that the case here? |
| 15:29 |
|
YuGiOhJCJ joined #minetest-dev |
| 15:36 |
|
Fixer joined #minetest-dev |
| 15:40 |
nerzhul |
because you miss something about cmake :D |
| 15:42 |
|
Fixer joined #minetest-dev |
| 15:47 |
|
troller joined #minetest-dev |
| 15:52 |
Unarelith |
nerzhul, what do you mean? |
| 16:01 |
Unarelith |
anyone knowns what's the purpose of this line? `LuaEntitySAO proto_LuaEntitySAO(NULL, v3f(0,0,0), "_prototype", "");` |
| 16:02 |
sfan5 |
context? |
| 16:04 |
Unarelith |
sfan5, https://github.com/minetest/minetest/blob/master/src/content_sao.cpp#L297 |
| 16:04 |
sfan5 |
looks unused to me |
| 16:05 |
rubenwardy |
it looks like it was originally intended to be an object that you copy and then deserialise into |
| 16:05 |
rubenwardy |
bad coding style, though |
| 16:07 |
Unarelith |
so it's unused? because it causes crash in my fork due to the usage of ServerActiveObject::registerType in LuaEntitySAO constructor |
| 16:07 |
Unarelith |
that's why this: `std::map<u16, ServerActiveObject::Factory> ServerActiveObject::m_types;` is in content_sao.cpp and not in serverobject.cpp |
| 16:08 |
Unarelith |
I'll remove it to fix the crash then |
| 16:10 |
Unarelith |
rubenwardy, about #7900, you said you didn't liked `engine` folder name, any better idea? |
| 16:10 |
ShadowBot |
https://github.com/minetest/minetest/issues/7900 -- Global code architecture improvement + SAO split by Quent42340 |
| 16:10 |
T4im |
if it registers itself form the constructor, you should probably assume it not being dead code |
| 16:10 |
rubenwardy |
src/client exists because things are starting to be partially moved into subfolders |
| 16:11 |
rubenwardy |
so I'm not sure why you can't move the rest of the client stuff into there |
| 16:14 |
Unarelith |
rubenwardy, ok then I'll merge `src/engine/client` into `src/client` |
| 16:15 |
rubenwardy |
sounds good |
| 16:39 |
Unarelith |
T4im, I understand your concern but everything seems to work after removing it |
| 16:40 |
Unarelith |
so it looks like legacy code which hasn't been touched since ages |
| 16:40 |
T4im |
:D |
| 16:41 |
Unarelith |
and if anything ever goes wrong I'll remember to try adding it back into the code first :p |
| 16:41 |
Unarelith |
rubenwardy, I don't think 80 column debate is relevant for #7899 |
| 16:41 |
ShadowBot |
https://github.com/minetest/minetest/issues/7899 -- Architectural improvement needed |
| 16:42 |
rubenwardy |
you did mention it in the OP |
| 16:42 |
rubenwardy |
maybe make a separate issue? |
| 16:44 |
Unarelith |
sure, but it was only a suggestion, if any debate on the topic is _really_ needed then I'll make another issue, but I think it could be better to discuss it here |
| 16:44 |
rubenwardy |
oh lol, found some random compat code |
| 16:44 |
rubenwardy |
nice |
| 16:46 |
Unarelith |
btw, should I let my PR in its current state or can I keep working on stuff? |
| 16:49 |
rubenwardy |
looks like a good first PR |
| 16:49 |
rubenwardy |
I'm currently trying to work out what proto_LuaEntitySAO is, and how the system works |
| 16:50 |
Unarelith |
I tried a lot of things, like creating a world, joining a server, and everything seems to work fine |
| 16:50 |
rubenwardy |
was added in this commit: https://github.com/minetest/minetest/commit/bfc68d31510bbd40732c19ada51d4683cb050de2 |
| 16:50 |
rubenwardy |
not very helpful |
| 16:51 |
rubenwardy |
there's also TestSAO proto_TestSAO(NULL, v3f(0,0,0)); |
| 16:52 |
Unarelith |
I removed it too |
| 16:55 |
Unarelith |
but maybe we should just ask this to celeron55? |
| 16:58 |
Unarelith |
hmm my travis build is failing but I don't know why |
| 16:59 |
rubenwardy |
you probably need to update the travis whitelist |
| 16:59 |
rubenwardy |
some old files are whitelisted by the code style litner |
| 16:59 |
rubenwardy |
ah, also builds |
| 17:00 |
rubenwardy |
looks like IRrlicht isn't included correctly |
| 17:00 |
Unarelith |
weird, I didn't make any change about Irrlicht |
| 17:01 |
Unarelith |
oh |
| 17:01 |
Unarelith |
I actually failed an unit test |
| 17:02 |
Unarelith |
testStreamRead failed |
| 17:03 |
rubenwardy |
it looks like the proto is used, weirdly, to register a factory to ServerActiveObject |
| 17:03 |
rubenwardy |
looks like it's called when a mapblock is loaded |
| 17:04 |
Unarelith |
so it's still needed somehow? |
| 17:04 |
rubenwardy |
so when testing, try spawning entities (ie: dropping an item), shutting down and restarting |
| 17:04 |
rubenwardy |
this is disgusting |
| 17:04 |
rubenwardy |
well, I guess it's one way of having code register itself without having to be called by somefunction |
| 17:04 |
rubenwardy |
as the code runs on init due to the constructor |
| 17:04 |
rubenwardy |
still a hack |
| 17:05 |
Unarelith |
well, I'll add it back then |
| 17:05 |
Unarelith |
will need another PR to fix that |
| 17:05 |
rubenwardy |
with the proto removed, you should get: "ServerEnvironment::activateObjects(): failed to create active object from static object" |
| 17:05 |
rubenwardy |
heh |
| 17:05 |
rubenwardy |
couldn't you just ammend it on? |
| 17:06 |
Unarelith |
I meant another PR to fix the need for this "thing" |
| 17:06 |
rubenwardy |
ah right, yeah |
| 17:07 |
Unarelith |
btw, is TestSAO still needed? |
| 17:08 |
Unarelith |
because it will be impossible to split TestSAO and LuaEntitySAO since they both need `m_types` declaration first: std::map<u16, ServerActiveObject::Factory> ServerActiveObject::m_types; |
| 17:08 |
Unarelith |
since this variable is used in `registerType` |
| 17:09 |
|
Fixer_ joined #minetest-dev |
| 17:09 |
rubenwardy |
isn't that just a forward declaration? |
| 17:09 |
rubenwardy |
wait, no |
| 17:09 |
Unarelith |
nope, it's the definition, the declaration is in serverobject.h, hmm or I could just add the protos to serverobject.cpp |
| 17:13 |
Unarelith |
so I added protos back to serverobject.cpp with a FIXME note indicating that their presence here is temporary and they need to move |
| 17:13 |
Unarelith |
[FAIL] testStreamRead - 0ms |
| 17:13 |
Unarelith |
[FAIL] testBufReader - 0ms |
| 17:13 |
Unarelith |
Unit Test Results: FAILED |
| 17:14 |
Unarelith |
though somehow I fail some unit tests |
| 17:17 |
Unarelith |
ok so actually minetest master branch fails the same tests |
| 17:18 |
rubenwardy |
lol |
| 17:18 |
rubenwardy |
unit tests are ran in the CI, though |
| 17:18 |
rubenwardy |
m |
| 17:18 |
rubenwardy |
oops, wrong window |
| 17:18 |
rubenwardy |
( alias m="make -j9" ) ;) |
| 17:19 |
rubenwardy |
works for me |
| 17:19 |
rubenwardy |
[PASS] testStreamRead - 0ms |
| 17:21 |
Unarelith |
hmmm weird, then why my build fails? :/ |
| 17:21 |
rubenwardy |
maybe make clean is needed? |
| 17:21 |
rubenwardy |
or it could be a platform issue |
| 17:21 |
rubenwardy |
doubgt it |
| 17:21 |
rubenwardy |
s/platform/undefined behaviour/g |
| 17:25 |
Unarelith |
I tried updating my system, I cleaned and restarted the build, we'll see |
| 17:27 |
Unarelith |
I still fail the same tests |
| 17:28 |
Unarelith |
Test assertion failed: readF1000(is) == 53.534f at test_serialization.cpp:305 |
| 17:28 |
Unarelith |
Test assertion failed: buf.getF1000() == 53.534f at test_serialization.cpp:472 |
| 17:30 |
rubenwardy |
well, probably fine then |
| 17:31 |
rubenwardy |
well, no |
| 17:32 |
rubenwardy |
#3943 |
| 17:32 |
rubenwardy |
lol |
| 17:32 |
ShadowBot |
https://github.com/minetest/minetest/issues/3943 -- testStreamRead and testBufReader failures |
| 17:33 |
Unarelith |
so it's a known thing for two years, ok x) |
| 17:35 |
|
Gael-de-Sailly joined #minetest-dev |
| 17:36 |
rubenwardy |
nerzhul: is it possible to restart this job? https://gitlab.com/minetest/minetest/-/jobs/125127331 |
| 17:36 |
rubenwardy |
and also answer your PMs ;) |
| 17:40 |
p_gimeno |
I'm interested in analyzing 3943. Where's the test that fails? |
| 17:41 |
p_gimeno |
ah, test_serialization.cpp:305, sorry |
| 17:47 |
p_gimeno |
Unarelith: if you change 53.534f to 53.534000396728515625f does it pass? |
| 17:48 |
p_gimeno |
(that particular test) |
| 17:49 |
Unarelith |
still doesn't pass |
| 17:49 |
p_gimeno |
hm, the problem is probably the opposite, 53.534f is correct but the incoming value is a double and is being compared as double |
| 17:49 |
p_gimeno |
thanks |
| 17:52 |
p_gimeno |
could you please try temporarily replacing line 305 with this and see if it still fails? {volatile float f = readF1000(is); UASSERT(f == 53.534f);} |
| 17:53 |
Unarelith |
still fails :/ |
| 17:53 |
p_gimeno |
wow |
| 17:55 |
p_gimeno |
could you please add this before the UASSERT and check if it's displayed? printf("readF1000=%a expected=%a\n", f, 53.534f); |
| 17:55 |
rubenwardy |
well, the issue is probably right in that it should be a range check |
| 17:55 |
Unarelith |
damn wait p_gimeno I may be have been stupid |
| 17:56 |
p_gimeno |
it's not easy to find a platform where it fails |
| 17:58 |
Unarelith |
p_gimeno, so I tried again you two suggestions to check if the error line actually changed, but without success |
| 17:59 |
p_gimeno |
could you try to get the output from printf, please? |
| 17:59 |
Unarelith |
p_gimeno, that's what I was doing :p |
| 18:00 |
Unarelith |
readF1000=0x1.ac45a4p+5 expected=0x1.ac45a2p+5 |
| 18:00 |
p_gimeno |
wow |
| 18:00 |
p_gimeno |
holy rounding error, Batman |
| 18:00 |
p_gimeno |
thank you :) |
| 18:01 |
|
Krock joined #minetest-dev |
| 18:02 |
p_gimeno |
the double result is 0x1.ac45a1cac0831p+5, so actually under 0x1.ac45a2p+5 yet the result is 1 ulp above |
| 18:02 |
* p_gimeno |
looks into readF1000 again |
| 18:10 |
p_gimeno |
ok, so the best explanation I have is that there's some optimization that is changing the division into a multiplication by the reciprocal |
| 18:11 |
|
Jordach joined #minetest-dev |
| 18:11 |
p_gimeno |
multiplying 53534.f by (float)(1.f/1000.f) can cause exactly that discrepancy |
| 18:15 |
p_gimeno |
Unarelith: any idea of the exact compiler flags used to compile src/util/serialize.cpp? |
| 18:15 |
p_gimeno |
(and what compiler?) |
| 18:16 |
Unarelith |
my compiler is gcc version 8.2.1 20180831 (GCC) |
| 18:16 |
Unarelith |
and I don't know for the compiler flags |
| 18:17 |
p_gimeno |
what processor/bits? |
| 18:18 |
Unarelith |
i7 7th gen/64 bits |
| 18:22 |
p_gimeno |
it's a very surprising result... I wonder if -ffast-math or some other unsafe math optimization flag is in effect |
| 18:22 |
rubenwardy |
I use an i7 8550U (ie: 8th gen, 64bit) |
| 18:22 |
celeron55 |
oh you figured out the AO factory thing on your own, good 8) |
| 18:23 |
nerzhul |
rubenwardy: why not |
| 18:23 |
celeron55 |
would have created some nasty bugs if it was removed without moving elsewhere |
| 18:23 |
rubenwardy |
from what I can see, it would've broken map loading from disk |
| 18:24 |
rubenwardy |
I really need a better IDE, it's really painful trying to navigate to symbols |
| 18:24 |
nerzhul |
rubenwardy clion |
| 18:24 |
rubenwardy |
maybe I should try that again :) |
| 18:24 |
nerzhul |
yeah don't do a massive refactor without understanding all |
| 18:24 |
nerzhul |
it's not as simple :p |
| 18:27 |
rubenwardy |
also, having classes only in .cpp files is perfectly valid |
| 18:27 |
nerzhul |
if they are intended to be private yes |
| 18:27 |
rubenwardy |
it's a classic example of implementation / interface |
| 18:27 |
rubenwardy |
which is heh |
| 18:29 |
rubenwardy |
the classes in game.cpp are disgusting though, and a bad example |
| 18:30 |
rubenwardy |
inputhandler should be in inputhandler.h (which actually exists) |
| 18:30 |
rubenwardy |
+already |
| 18:32 |
Krock |
to be fair, game.cpp probably never had a major code movement, resulting in partially messy code from "the old days" |
| 18:33 |
rubenwardy |
yeah |
| 18:33 |
rubenwardy |
I really want to work on an input rework at some point, but I always feel a bit sick after trying |
| 18:33 |
rubenwardy |
partially Irrlicht's fault, tbh |
| 18:35 |
rubenwardy |
tbh, I do too much talking and not enough actual coding |
| 18:37 |
rubenwardy |
merging #7863 in 10 |
| 18:37 |
ShadowBot |
https://github.com/minetest/minetest/issues/7863 -- Don't display page-nav buttons when #store.packages == 0 by ClobberXD |
| 18:37 |
Krock |
if I were just 30s earlier... |
| 18:37 |
rubenwardy |
looool |
| 18:38 |
rubenwardy |
you can if you like |
| 18:38 |
Krock |
nah, go on ;) |
| 18:38 |
rubenwardy |
I'll probably forget anyway, I usually do |
| 18:43 |
rubenwardy |
#7901 |
| 18:43 |
ShadowBot |
https://github.com/minetest/minetest/issues/7901 -- Accepting PRs/patches from non-Github users |
| 18:43 |
rubenwardy |
p_gimeno ^ |
| 18:45 |
p_gimeno |
sweet :) |
| 18:45 |
rubenwardy |
updated |
| 18:46 |
rubenwardy |
mergin... |
| 18:47 |
rubenwardy |
done |
| 18:49 |
p_gimeno |
I like the bot approach, if it's able to fetch any remote branch on any repo no matter where it's hosted |
| 18:50 |
rubenwardy |
that's part of the issue |
| 18:50 |
ANAND |
Thanks :) |
| 18:50 |
rubenwardy |
the bot would need to have it's own fork and update the branches, in that case |
| 18:50 |
rubenwardy |
it's probably not the easiest solution |
| 18:51 |
p_gimeno |
sounds like quite some work, yeah |
| 18:53 |
p_gimeno |
I'm willing to do it on GitLab |
| 18:54 |
rubenwardy |
that's probably the easiest method out of the ones given |
| 18:54 |
rubenwardy |
well, idk |
| 18:58 |
Krock |
Unarelith: thanks for #7902. Makes things easier. What's "testbm.txt" btw? |
| 18:58 |
ShadowBot |
https://github.com/minetest/minetest/issues/7902 -- Client-specific files moved to 'src/client' by Quent42340 |
| 18:59 |
Unarelith |
Krock, np, and "testbm.txt" is a file sometimes generated by MT build |
| 19:00 |
Krock |
weird. will check where this comes from |
| 19:00 |
Krock |
ah. perhaps just ignore the entire test world |
| 19:01 |
Krock |
src/unittest/test_world/world.mt -> src/unittest/test_world/ |
| 19:02 |
Unarelith |
the file is generated in the repo root |
| 19:02 |
p_gimeno |
I can only reproduce a reduced version of #3943 <https://paste.scratchbook.ch/view/69368d2b>, if I activate either -ffast-math or -funsafe-math-optimizations, trying more FP-related flags... |
| 19:02 |
ShadowBot |
https://github.com/minetest/minetest/issues/3943 -- testStreamRead and testBufReader failures |
| 19:02 |
Krock |
oh, that's not related to the test world, more as a side-product of the unittests overall |
| 19:02 |
Unarelith |
I guess so |
| 19:03 |
Krock |
test_ban.cpp apparently does not delete that file after finishing the tests |
| 19:05 |
Unarelith |
Krock, for file naming, is my suggestion open to discussion? |
| 19:05 |
Krock |
yes sure. was just my personal opinion on keeping it consistent with the existing code base |
| 19:06 |
Krock |
also personal preference |
| 19:06 |
Unarelith |
it's mine too, purely subjective, but I have arguments to defend it :p |
| 19:07 |
rubenwardy |
I'd prefer names which match class names |
| 19:08 |
rubenwardy |
+ where relevant |
| 19:08 |
Krock |
there used to be meetings on Saturday around 9 PM UTC where most of the devs were online. Too bad such discussions are now much more luck-based and spontaneous |
| 19:08 |
rubenwardy |
this could be left until later, though |
| 19:09 |
Krock |
btw, the Android source list is in build/android/jni/Android.mk |
| 19:31 |
rubenwardy |
oh nice, you made your own MC-like |
| 19:37 |
Unarelith |
Krock, rubenwardy, why you don't use automatic .cpp file gathering instead of having them manually entered into Android.mk AND CMakeLists.txt? |
| 19:37 |
Unarelith |
it's so inconvenient |
| 19:38 |
|
ANAND joined #minetest-dev |
| 19:38 |
rubenwardy |
because the Android build system is completely shit |
| 19:38 |
sfan5 |
android.mk is a hack, it should not exist |
| 19:38 |
rubenwardy |
^ |
| 19:38 |
sfan5 |
android should use cmake like any other platform |
| 19:39 |
rubenwardy |
sfan5 was working on a nice cmake build system because this hacky shit by sapier was merged |
| 19:39 |
rubenwardy |
there's a PR for this by nerzhul |
| 19:39 |
sfan5 |
nrz you mean |
| 19:39 |
Unarelith |
it's still possible to do this inside Android.mk |
| 19:39 |
sfan5 |
my cmake build system predates sapier's android port |
| 19:40 |
sfan5 |
Unarelith: either way I wouldn't worry too much about android right now when developing |
| 19:40 |
rubenwardy |
that's what I was referring to. Did you stop working on it before sapier started porting? |
| 19:40 |
rubenwardy |
and yeah, you could still use find |
| 19:40 |
sfan5 |
i stopped working on it eventually when sapier's port was nearing completion |
| 19:40 |
sfan5 |
(because his stuff worked and mine didn't at that point) |
| 19:41 |
rubenwardy |
thought so |
| 19:41 |
rubenwardy |
#7123 |
| 19:41 |
ShadowBot |
https://github.com/minetest/minetest/issues/7123 -- Android: switch to cmake based standard builds by nerzhul |
| 19:43 |
Unarelith |
sfan5, the only problem is keeping Android.mk valid, which is a pain due to all the files I moved |
| 19:46 |
Unarelith |
rubenwardy, Android.mk updated in both PRs |
| 19:47 |
rubenwardy |
nice, thanks |
| 19:49 |
Unarelith |
rubenwardy, what was the issue with mainmenu? |
| 19:50 |
rubenwardy |
well, gui contains stuff which is also used by the main menu |
| 19:50 |
rubenwardy |
so it depends whether you mean client as the state of being, or client as the build target |
| 19:50 |
rubenwardy |
like, should main menu stuff be in client? |
| 19:50 |
Krock |
I'd assume "client" is the in-game client |
| 19:51 |
rubenwardy |
in which case src/gui should stay separate |
| 19:51 |
rubenwardy |
but it shouldn't have references to src/client in it, really |
| 19:51 |
Krock |
well, both mainmenu and in-game client need the gui |
| 19:52 |
|
troller joined #minetest-dev |
| 19:52 |
Unarelith |
in my opinion, the code should be separated between 'client', 'server' and 'common' |
| 19:54 |
Krock |
so GUI would be common..? |
| 19:54 |
Unarelith |
that would be weird indeed |
| 19:54 |
rubenwardy |
GUI isn't used by server, though |
| 19:55 |
Unarelith |
then it should go into client |
| 19:55 |
rubenwardy |
client would be the client target in this case, rather than the client in the server-client relationship |
| 19:55 |
Unarelith |
it would be, and maybe a new folder `client/game/` could be the in-game client |
| 19:56 |
rubenwardy |
maybe |
| 19:56 |
rubenwardy |
that could work |
| 19:56 |
rubenwardy |
but game is also a bad term |
| 19:56 |
Unarelith |
sure, I don't have a better name yet |
| 19:57 |
|
ssieb joined #minetest-dev |
| 20:01 |
nerzhul |
Unarelith i had a such opinion 2 years ago |
| 20:01 |
nerzhul |
GUI is client only |
| 20:01 |
nerzhul |
i think we need common & client only |
| 20:02 |
nerzhul |
let me push some little reorganization PR |
| 20:02 |
rubenwardy |
err, please don't conflict with each other XD |
| 20:02 |
nerzhul |
i will split this in multiple parts to ensure we can do an easy bissect if needed |
| 20:03 |
rubenwardy |
have you seen Unarelith's PRs, nerzhul? |
| 20:04 |
nerzhul |
i will look |
| 20:04 |
Unarelith |
<rubenwardy> oh nice, you made your own MC-like <= I missed it :O |
| 20:05 |
rubenwardy |
sorry, I stalked you briefly |
| 20:05 |
nerzhul |
hmm they seems to be very big |
| 20:05 |
|
Foz joined #minetest-dev |
| 20:05 |
rubenwardy |
well yeah, there's a lot of files |
| 20:05 |
rubenwardy |
luckly the client one won't break much because git can handle files changing location |
| 20:05 |
nerzhul |
i don't agree with .hpp |
| 20:05 |
Unarelith |
rubenwardy, np haha, this is a pretty old project I tried to revive a few months ago |
| 20:05 |
rubenwardy |
:( |
| 20:05 |
nerzhul |
.h is our convention |
| 20:05 |
Unarelith |
nerzhul, makes no sense |
| 20:05 |
rubenwardy |
I think we can change to .hpp though |
| 20:06 |
Unarelith |
.hpp is for C++, .h is for C |
| 20:06 |
rubenwardy |
just because .h is currently what we use |
| 20:06 |
nerzhul |
we are not using this |
| 20:06 |
rubenwardy |
and yeah, what Unarelith |
| 20:06 |
nerzhul |
and we will not use |
| 20:06 |
rubenwardy |
+Said |
| 20:06 |
nerzhul |
.hpp is not very common in C++ |
| 20:06 |
rubenwardy |
really? |
| 20:06 |
nerzhul |
look at stlibc++ |
| 20:06 |
nerzhul |
haha |
| 20:06 |
nerzhul |
.h everytime |
| 20:06 |
nerzhul |
it's used, but not everywhere |
| 20:06 |
Unarelith |
"look at something really old, see, I'm right" |
| 20:07 |
rubenwardy |
we currently have a .gitattributes file to tell Github that .h is for C++ |
| 20:07 |
Unarelith |
.hpp is for modern C++, to make the distinction between .h and .hpp |
| 20:07 |
Krock |
well, in a certain way he's right |
| 20:07 |
nerzhul |
we will not change this currently |
| 20:07 |
rubenwardy |
we also use #pragma once, which is probably not as widely used and is *non-standard* |
| 20:08 |
rubenwardy |
but anyway, I suggest leaving it out of that PR |
| 20:08 |
nerzhul |
i see this in the gnome C code |
| 20:08 |
nerzhul |
yeah don't change convention right now |
| 20:08 |
Unarelith |
nerzhul, https://stackoverflow.com/questions/152555/h-or-hpp-for-your-class-definitions |
| 20:08 |
p_gimeno |
I think I've seen more .h than .hpp for C++ code |
| 20:09 |
rubenwardy |
I'm actually surprised at this, would've thought that the separate extension for C++ would've been more common |
| 20:09 |
Unarelith |
same for me, but seems like .h is still the default in IDE these days so people don't bother change it |
| 20:10 |
nerzhul |
yep |
| 20:10 |
|
Foz joined #minetest-dev |
| 20:11 |
nerzhul |
it's okay except the file names for the SAO thing |
| 20:13 |
nerzhul |
for the client move, ty git |
| 20:13 |
nerzhul |
i commented what i wanted for inclusion |
| 20:14 |
nerzhul |
i don't think we should add -I flag on server to include client files |
| 20:14 |
rubenwardy |
I agree with that |
| 20:14 |
rubenwardy |
I imagine it's a stepping stone |
| 20:14 |
nerzhul |
yes but error prone in IDE |
| 20:14 |
nerzhul |
i prefer explicit fix |
| 20:14 |
rubenwardy |
I agree with not adding the -I flag * |
| 20:14 |
rubenwardy |
is what I meant |
| 20:15 |
nerzhul |
it's added with the include_directories(.../client) |
| 20:15 |
nerzhul |
fix the headers |
| 20:15 |
nerzhul |
it will permit to track client code in common files if we find it |
| 20:15 |
nerzhul |
the include_directories should be in the client/CMakeLists.txt part |
| 20:16 |
nerzhul |
but later |
| 20:16 |
nerzhul |
be explicit at now |
| 20:16 |
Unarelith |
nerzhul, ok then I'll do it, but it will take a LOT of time since there's some files I don't even compile on my system, which are only checked by a 15 min long travis build |
| 20:16 |
nerzhul |
wtf |
| 20:16 |
rubenwardy |
oh really |
| 20:16 |
nerzhul |
why don't you compile ? |
| 20:17 |
rubenwardy |
separate target? |
| 20:17 |
Unarelith |
I'm building on ArchLinux, without postgresql, without windows-specific files, etc... |
| 20:17 |
rubenwardy |
ah ofc |
| 20:17 |
|
Foz joined #minetest-dev |
| 20:17 |
nerzhul |
i'm building on archlinux too |
| 20:17 |
nerzhul |
with postgresql but without windows |
| 20:17 |
Unarelith |
Moreover some include directives are not needed on my PC but needed for travis |
| 20:17 |
nerzhul |
i'm using clion too |
| 20:18 |
Unarelith |
I'm using vim tho |
| 20:18 |
nerzhul |
omg |
| 20:18 |
nerzhul |
use qtcreator at least if you don't have a licensed thing |
| 20:18 |
nerzhul |
or gnome builder |
| 20:18 |
Unarelith |
._. |
| 20:18 |
nerzhul |
C++ without good IDE is dangerous :p |
| 20:18 |
Unarelith |
I'll never switch from vim dude, why would I ever use anything else? |
| 20:19 |
nerzhul |
use emacs *jokeù |
| 20:19 |
jacky |
lol |
| 20:19 |
nerzhul |
(i don't use emacs but vim) |
| 20:19 |
nerzhul |
cmake integration in IDE is good |
| 20:19 |
jacky |
yeah vim + cmake + good tests and you're good |
| 20:19 |
nerzhul |
and it's easy on archlinux to compile |
| 20:19 |
nerzhul |
pacman -S cmake base-devel clang |
| 20:19 |
nerzhul |
mkdir cmakebuild && cmakebuild && cmake .. -DRUN_IN_PLACE=1 && make |
| 20:19 |
rubenwardy |
vim's symbol jumping with ctags is pretty aweosme |
| 20:20 |
nerzhul |
i don't know about that |
| 20:20 |
Unarelith |
rubenwardy, I'm also using YouCompleteMe for code completion |
| 20:20 |
rubenwardy |
the issue is that it's vim, however |
| 20:20 |
nerzhul |
then i will go look for marvel agents of shield, see you in 1 or 2 hours |
| 20:20 |
rubenwardy |
ooh nice, I like that show |
| 20:20 |
nerzhul |
please remove the include_directories and fix headers :p |
| 20:20 |
rubenwardy |
lol |
| 20:30 |
|
YuGiOhJCJ joined #minetest-dev |
| 20:50 |
|
troller joined #minetest-dev |
| 20:56 |
|
Icedream joined #minetest-dev |
| 21:04 |
Unarelith |
nerzhul, I made the changes you requested, though #7902 may still not build on travis due to missing fixes |
| 21:04 |
ShadowBot |
https://github.com/minetest/minetest/issues/7902 -- Client-specific files moved to 'src/client' by Quent42340 |
| 21:08 |
|
Icedream joined #minetest-dev |
| 21:21 |
|
sys4 joined #minetest-dev |
| 21:24 |
|
fireglow joined #minetest-dev |
| 21:29 |
|
paramat joined #minetest-dev |
| 21:31 |
paramat |
nerzhul is #7820 mergeable? it seems to be running ok for those who tested. is that decremented loop the only issue? the indents have been changed back to tabs |
| 21:31 |
ShadowBot |
https://github.com/minetest/minetest/issues/7820 -- Update Android java-part by MoNTE48 |
| 21:32 |
paramat |
also everyone, #7850 just needs 1 more approval |
| 21:33 |
ShadowBot |
https://github.com/minetest/minetest/issues/7850 -- Make non-formspec modal menus respect gui scale by stujones11 |
| 21:37 |
paramat |
as does #7395 |
| 21:37 |
ShadowBot |
https://github.com/minetest/minetest/issues/7395 -- Add Lua methods 'set_rotation()' and 'get_rotation()' by CoderForTheBetter |
| 21:46 |
|
troller joined #minetest-dev |
| 21:55 |
nerzhul |
merging #7850 |
| 21:55 |
ShadowBot |
https://github.com/minetest/minetest/issues/7850 -- Make non-formspec modal menus respect gui scale by stujones11 |
| 22:01 |
paramat |
:D |
| 22:02 |
|
fireglow joined #minetest-dev |
| 22:06 |
|
Jordach joined #minetest-dev |
| 22:07 |
paramat |
down to 5 blockers |
| 22:09 |
paramat |
actually 6 |
| 22:20 |
Unarelith |
paramat, #7395 seems to be the only one of these 4 PRs which could create conflicts with my PRs: https://github.com/minetest/minetest/pulls?q=is%3Aopen+is%3Apr+label%3ABlocker |
| 22:20 |
ShadowBot |
https://github.com/minetest/minetest/issues/7395 -- Add Lua methods 'set_rotation()' and 'get_rotation()' by CoderForTheBetter |
| 22:21 |
Unarelith |
#7891 and #7820 could too but on a really small scale (.gitignore, l_mainmenu.cpp) |
| 22:21 |
ShadowBot |
https://github.com/minetest/minetest/issues/7891 -- Fix ContentDB packages timing out by using download_file instead by rubenwardy |
| 22:21 |
ShadowBot |
https://github.com/minetest/minetest/issues/7820 -- Update Android java-part by MoNTE48 |
| 22:21 |
Unarelith |
But you mentioned 6 blockers so I don't know for the other two |
| 22:24 |
Unarelith |
*sorry I meant #7768 not 7820 |
| 22:24 |
ShadowBot |
https://github.com/minetest/minetest/issues/7768 -- Network: Send IEEE floats by SmallJoker |
| 22:31 |
paramat |
ok. sorry by '6' i mean including 2 issues |
| 22:32 |
Unarelith |
oh ok |
| 22:32 |
paramat |
i'll push to get those blockers merged soon, especially 7395 |
| 22:44 |
|
CoderForTheBette joined #minetest-dev |
| 22:44 |
|
CoderForTheBette left #minetest-dev |
| 22:49 |
|
Devy joined #minetest-dev |
| 22:52 |
|
Devy left #minetest-dev |
| 22:52 |
|
CoderForTheBette joined #minetest-dev |
| 22:54 |
|
CoderForTheBette left #minetest-dev |
| 22:57 |
|
CoderForTheBette joined #minetest-dev |
| 22:59 |
CoderForTheBette |
Yeah, we should get #7395 merged so I don't have to rebase it (again). I have used my PR in my own branch and have not noticed anything weird or any extra lag with it. If there are any questions just post it on the Github PR, I'm still around! |
| 22:59 |
ShadowBot |
https://github.com/minetest/minetest/issues/7395 -- Add Lua methods 'set_rotation()' and 'get_rotation()' by CoderForTheBetter |
| 23:05 |
p_gimeno |
hm, I'm checking 7395 and got some problems with it |
| 23:06 |
CoderForTheBette |
What seems to be the problems? |
| 23:08 |
p_gimeno |
I am looking closer, but I'm not sure the player is correctly marked dirty |
| 23:09 |
CoderForTheBette |
Ah, that could be a problem! Good catch, I'll take a look at the code right now to help verify that is something that might happen. |
| 23:09 |
p_gimeno |
also, it would be best if accompanied by #7768, to add precision |
| 23:09 |
ShadowBot |
https://github.com/minetest/minetest/issues/7768 -- Network: Send IEEE floats by SmallJoker |
| 23:10 |
CoderForTheBette |
This could also be a problem that was occurring before my PR. |
| 23:11 |
p_gimeno |
yes, it's independent |
| 23:12 |
CoderForTheBette |
For sure, let's see who gets their PR merged first. However, if it comes to it then I do not think rebasing mine would be too hard. I think it also would not be too hard to rebase that one as well. Which ever comes first. |
| 23:13 |
p_gimeno |
who does the heavy lifting? I don't see .X used in the PR, for instance |
| 23:14 |
p_gimeno |
I mean, I'm trying to understand how these angles are used, but I don't see anything in the PR, is it because that was pre-existing code? |
| 23:16 |
CoderForTheBette |
This was semi existing code that I had to modify to use the vector3df type instead of just a float. |
| 23:16 |
CoderForTheBette |
Let me see if I can get a path of what happens when the added lua methods are called to where the rotation takes place. |
| 23:17 |
p_gimeno |
also, are the internal values in degrees??? |
| 23:17 |
p_gimeno |
because otherwise this doesn't look right to me: +float yaw = co->getRotation().Y * core::DEGTORAD; |
| 23:17 |
CoderForTheBette |
Yes, it has always been like that since Irrlicht uses that. You can look at the old code and it does the same thing. |
| 23:17 |
p_gimeno |
eek |
| 23:18 |
CoderForTheBette |
I know, gross. |
| 23:18 |
|
fwhcat joined #minetest-dev |
| 23:19 |
CoderForTheBette |
In fact, part of my PR preserves the old setYaw() methods so here is exactly where this "old" code is: https://github.com/minetest/minetest/pull/7395/files#diff-9444313da8be4290e9304a4b1faed804R935 |
| 23:21 |
CoderForTheBette |
Remember, the units go in as radians, get converted to degrees, and then if getYaw() or getRotation() is called they are converted back to radians from degrees. |
| 23:21 |
p_gimeno |
thanks |
| 23:23 |
CoderForTheBette |
Anyway, back to this set the player dirty stuff. I don't see where anything changed when it comes to this. If you have a scenario/proof that something is wrong then please tell us! |
| 23:25 |
p_gimeno |
is this where it's passed to the engine?node->setRotation(rot); |
| 23:26 |
CoderForTheBette |
yes, right here: https://github.com/minetest/minetest/pull/7395/files#diff-722710b5ecd9db42e7dd785dd517e186R774 |
| 23:26 |
CoderForTheBette |
oh, one line lower. |
| 23:26 |
p_gimeno |
yes, thanks |
| 23:28 |
p_gimeno |
right there, why obtain rot two lines above? |
| 23:28 |
p_gimeno |
it made sense when only Y was changed, but now it's completely replaced |
| 23:29 |
CoderForTheBette |
hm |
| 23:29 |
CoderForTheBette |
Yeah, no sense in doing that. |
| 23:29 |
CoderForTheBette |
Line 773, right? |
| 23:29 |
p_gimeno |
right |
| 23:30 |
p_gimeno |
I'm also a bit puzzled by the minus sign |
| 23:30 |
CoderForTheBette |
Doesn't seem necessary. |
| 23:30 |
CoderForTheBette |
not the minus sign, sorry. |
| 23:31 |
p_gimeno |
yeah, sorry, I'm firing too many comments in a row :) |
| 23:32 |
CoderForTheBette |
I honestly forget why the minus sign is needed. I do remember that if I got rid of it then something did not work. This was months ago however. I'll do some more investigating real quick. |
| 23:34 |
p_gimeno |
with MT's weird axis convention and left-handed reference frame, I wouldn't be surprised if only one of these needs a minus sign |
| 23:34 |
paramat |
i'll mark the PR WIP for now then |
| 23:35 |
Unarelith |
I'm working on using `file(GLOB *.cpp)` in CMake instead of manually writing each filename, but I have an issue with `src/settings_translation_file.cpp` since it's not a real C++ file |
| 23:36 |
Unarelith |
can I move this file? will it cause hundreds of changes in .po files? |
| 23:37 |
Unarelith |
I don't know anything about gettext |
| 23:37 |
CoderForTheBette |
Weird, Minetest was already obtaining 'rot' before but it was only a single axis: https://github.com/minetest/minetest/blob/master/src/content_cao.cpp#L738 |
| 23:38 |
CoderForTheBette |
wait, not single axis |
| 23:38 |
CoderForTheBette |
it was all three, but from the engine |
| 23:38 |
p_gimeno |
yes, it got all three, changed Y, set all three (IIUC) |
| 23:39 |
CoderForTheBette |
But now that should not be a problem since all three will be stored and can just be set each time without getting the engine's. That is, along as the rotation is not modified anywhere else in MInetest. |
| 23:41 |
CoderForTheBette |
Ehhh, forget that last bit. I'm pretty sure that it is just completely unnecessary. |
| 23:41 |
p_gimeno |
yep |
| 23:42 |
CoderForTheBette |
Ok, Ill get rid of that line by just defining a v3f and let it use its default constructor to set it to 0,0,0 |
| 23:53 |
p_gimeno |
interpolating in Euler is also yucky, but I think that given the use cases for interpolation, a proper slerp is not necessary |
| 23:57 |
CoderForTheBette |
Yeah, I don't like the way I'm doing it in this PR, but I think someone requested it. It does look good. I extended the code (again) that was in place for interpolating a single axis. |
