| Time |
Nick |
Message |
| 00:07 |
|
jin_xi joined #minetest-dev |
| 00:36 |
|
Player_2 joined #minetest-dev |
| 01:20 |
|
shadowzone joined #minetest-dev |
| 01:24 |
|
shadowzone joined #minetest-dev |
| 01:28 |
|
crazyR joined #minetest-dev |
| 02:04 |
|
gnatt joined #minetest-dev |
| 02:25 |
|
compunerd joined #minetest-dev |
| 02:37 |
|
shadowzone joined #minetest-dev |
| 02:37 |
|
Miner_48er joined #minetest-dev |
| 02:50 |
|
psedlak joined #minetest-dev |
| 02:50 |
|
Taoki joined #minetest-dev |
| 03:13 |
|
kahrl joined #minetest-dev |
| 03:38 |
|
everamzah joined #minetest-dev |
| 03:54 |
|
Zeno` joined #minetest-dev |
| 03:59 |
|
Megal_ joined #minetest-dev |
| 04:39 |
|
werwerwer joined #minetest-dev |
| 04:39 |
|
Hunterz joined #minetest-dev |
| 04:46 |
|
chchjesus joined #minetest-dev |
| 04:58 |
Zeno` |
very odd commit message for last addition to master lol |
| 04:59 |
Zeno` |
it's like a tweet |
| 05:01 |
Zeno` |
https://gist.github.com/Zeno-/960b82defa1f68645331 |
| 05:02 |
Zeno` |
profiling now but it "feels" faster (also solves the changing a global value to solve a local problem issue), as discussed yesterday |
| 05:09 |
Zeno` |
getting 10 fps in valgrind with instrumentation on :D |
| 05:15 |
Zeno` |
That's a bit better than 1-3 |
| 05:24 |
Zeno` |
#2281 for feedback please |
| 05:24 |
ShadowBot |
https://github.com/minetest/minetest/issues/2281 -- Remove shader workaround in itemdef.cpp by Zeno- |
| 05:35 |
kahrl |
Zeno`: lgtm |
| 05:39 |
Zeno` |
will merge in about an hour then. Thanks kahrl |
| 05:39 |
Zeno` |
Just doing some more tests and profiling in the meantime |
| 05:50 |
|
crazyR joined #minetest-dev |
| 05:57 |
Zeno` |
will also merge 2164. I can't test on Hurd, but will just have to trust apoleon that it fixes the Hurd issue |
| 06:01 |
|
chrisf joined #minetest-dev |
| 06:27 |
|
nrzkt joined #minetest-dev |
| 06:30 |
nrzkt |
hi |
| 06:32 |
nrzkt |
i fixed the android build |
| 06:32 |
nrzkt |
#2277 |
| 06:32 |
ShadowBot |
https://github.com/minetest/minetest/issues/2277 -- Main.cpp cleanup: Move ClientLauncher and InputHandlers to dedicated files by nerzhul |
| 06:35 |
nrzkt |
i think we must use subdirectories for sources, this PR add src/client and src/shared for client sources and shared sources, this will permit to have a better view of the project, and to get a separation between client, server and shared sources. I think we need, in the future, migrate some common code to a shared lib, it's better for compilation time and project overview/maintenance. |
| 06:38 |
hmmmm |
i don't quite like that idea... |
| 06:38 |
nrzkt |
why ? |
| 06:38 |
hmmmm |
there are already many subdirectories |
| 06:38 |
nrzkt |
at this time all sources are in a common directory, it's a pure bazaar :) |
| 06:39 |
hmmmm |
just adding more might make things more confusing |
| 06:39 |
|
Hunterz joined #minetest-dev |
| 06:39 |
nrzkt |
if they are per theme, for example: gui, network, graphics, client, server |
| 06:39 |
hmmmm |
and make a common library is bad because then whole program optimization wouldn't work as well |
| 06:40 |
hmmmm |
i like organization just as anybody else, but it's a slippery slope |
| 06:40 |
hmmmm |
i don't want minetest to end up having a source tree like tersasology |
| 06:40 |
nrzkt |
for network merge i already create a network directory, because there is many sources. But i don't like java directories, then 1 or 2 subdirs is the maximum :) |
| 06:40 |
hmmmm |
(i realize they take it to an absurd extreme and it's not normal, but I can see overzealous organization doing that) |
| 06:41 |
hmmmm |
also obviously this would break ALL pending pull requests |
| 06:41 |
hmmmm |
this is the largest issue here |
| 06:42 |
hmmmm |
I know you're wanting to make really BIG changes but you need to be mindful of all the issues they could cause instead of only focusing on positives |
| 06:42 |
hmmmm |
maybe if the number of active pull requests is reduced to a certain acceptable level first |
| 06:43 |
nrzkt |
of course, it's why the split must be done step by step. in #2277 i only move some code and there is only one conflicting PR, then it's possible, i don't say we must move all code today, but move it when we refactor some things, step by step may be good |
| 06:43 |
ShadowBot |
https://github.com/minetest/minetest/issues/2277 -- Main.cpp cleanup: Move ClientLauncher and InputHandlers to dedicated files by nerzhul |
| 06:43 |
hmmmm |
but please... don't do the common shared library thing... it'd ruin global optimizations |
| 06:43 |
nrzkt |
no problem, it's just an idea |
| 06:44 |
hmmmm |
which is the pull request that conflicted? |
| 06:44 |
nrzkt |
hmm i can't remember and i msut go to work, it's a move on timegetter in main.cpp because my issue move the same thing :) |
| 06:44 |
hmmmm |
well, if it's your own PR then I see no problem |
| 06:44 |
nrzkt |
see you |
| 06:44 |
hmmmm |
but you need to be curteous to other contributers |
| 06:44 |
hmmmm |
contributors* |
| 06:45 |
nrzkt |
no, the other pr is not mine, but i can discuss with contributor, i'll speak with him today, promice |
| 06:45 |
nrzkt |
i must go :) |
| 06:45 |
hmmmm |
okay |
| 06:45 |
hmmmm |
we really need to merge PRs |
| 06:46 |
hmmmm |
maybe tomorrow I'll take a break from work and focus on minetest all day long |
| 06:46 |
Zeno` |
sounds... riveting |
| 06:47 |
hmmmm |
i sware i have ADD, i'm only able to focus on one coding project at a time |
| 06:49 |
Zeno` |
I do like the idea of a limited number of subdirectories but as I mentioned in the PR I think it needs discussion |
| 06:50 |
Zeno` |
e.g. for the points you raised |
| 06:50 |
Zeno` |
I think the naming (taxonomy) must be agreed upon as well if it goes ahead |
| 06:51 |
Zeno` |
e.g. I don't like src/shared/ (it makes me think of a library or something) and would prefer src/common/ |
| 06:51 |
hmmmm |
how about leave the common things in the main directory and put client-specific things in client and server-things in server |
| 06:51 |
Zeno` |
could take years... just like botanists spend years moving plant names around :D |
| 06:51 |
hmmmm |
i don't understand the point of having the main directory contain nothing but subdirectories |
| 06:52 |
Zeno` |
I don't understand that either |
| 06:52 |
hmmmm |
it's a java thing |
| 06:52 |
hmmmm |
I start to sneeze when I see people attempting to inject java-isms into minetest |
| 06:52 |
Zeno` |
so, yeah, your idea is better. the subdirectories should be based on clearly separable "sub 'modules'/'systems'" |
| 06:53 |
hmmmm |
because java isn't just a language, it's an entire mindset |
| 06:53 |
Zeno` |
I can never browse java projects |
| 06:53 |
hmmmm |
and a disasterous one at that |
| 06:53 |
Zeno` |
it's ridiculous... you spend an hour traversing 1000 directories to find it contains one function and then you backtrack heh |
| 06:54 |
Zeno` |
the function 1000 levels deep is probably 3 lines long as well |
| 06:54 |
sofar |
problems with leaves not showing up properly in the inventory? just pulled git... |
| 06:54 |
* Zeno` |
looks |
| 06:55 |
sofar |
yeah, some leaves in inventoryplus are only half visible, others are completely gone |
| 06:55 |
Zeno` |
they seem to be working for me |
| 06:55 |
Zeno` |
hmm |
| 06:55 |
sofar |
like the texture moved left |
| 06:55 |
Zeno` |
and it just started happening? |
| 06:56 |
sofar |
just did a git pull and build |
| 06:56 |
sofar |
can't say I ever saw it before |
| 06:56 |
sofar |
it's with an 0.4.11 server tho |
| 06:57 |
Zeno` |
screenshot? |
| 06:58 |
Zeno` |
also, is inventoryplus up to date? |
| 06:58 |
Zeno` |
actually |
| 06:58 |
Zeno` |
hmm |
| 07:01 |
Zeno` |
sofar, update git again please? |
| 07:10 |
|
jin_xi joined #minetest-dev |
| 07:15 |
|
kilbith joined #minetest-dev |
| 07:34 |
Zeno` |
ok, we re-merge #2281 shortly |
| 07:34 |
ShadowBot |
https://github.com/minetest/minetest/issues/2281 -- Remove shader workaround in itemdef.cpp by Zeno- |
| 07:34 |
Zeno` |
err s/we/will |
| 07:36 |
Zeno` |
Anything you'd like me to add to the commit message, Tesseract? |
| 07:38 |
sofar |
Zeno`: sorry, was afk a sec |
| 07:38 |
sofar |
pulling |
| 07:38 |
Tesseract |
Zeno`: Nope, seems fine. |
| 07:41 |
sofar |
Zeno`: all good now, all leaves show up fine in the inventory :D |
| 07:42 |
Zeno` |
sofar, thanks for bringing it up; was a (sort of) typo on my part (not really a typo, a 10 char change that I meant to add and missed... oops) |
| 07:43 |
sofar |
that's why I like running git... I'm a software developer myself and I only have 2 eyes.... |
| 07:47 |
Zeno` |
sofar, sometimes even 6 eyes miss the small things... :D |
| 07:48 |
Zeno` |
sofar, if you feel inclined you may want to test the latest git (again) |
| 07:48 |
sofar |
I usually pull once a week or so |
| 07:48 |
sofar |
especially around release time ;) |
| 07:50 |
|
nrzkt joined #minetest-dev |
| 07:50 |
Zeno` |
I'm glad that we were talking about git and context is available |
| 07:53 |
hmmmm |
#justaustraliathings |
| 07:55 |
nrzkt |
hmmmm: okay with #2277, then ? i won't move every file and create a library, source move only allowed when refactor ? |
| 07:55 |
ShadowBot |
https://github.com/minetest/minetest/issues/2277 -- Main.cpp cleanup: Move ClientLauncher and InputHandlers to dedicated files by nerzhul |
| 07:56 |
hmmmm |
ermm.. sure, but why don't you wait until some PRs are cleared out |
| 07:56 |
nrzkt |
i looked at PR and i haven't found the PR i mentioned yet, it seems it is very old |
| 08:00 |
nrzkt |
hmmmm: what about #2267 ? |
| 08:00 |
ShadowBot |
https://github.com/minetest/minetest/issues/2267 -- Mgv7: Add lower limit of zero to mountain height noise by paramat |
| 08:01 |
hmmmm |
i haven't looked at that myself but i trust paramat's judgement |
| 08:01 |
hmmmm |
at least we know it's not going to cause minetest to implode |
| 08:02 |
nrzkt |
no, it only add MYMAX then the height could not be zero. But is the Perlin2D only generate negative numbers the map will be a flat zone |
| 08:02 |
nrzkt |
could not be under zero* |
| 08:06 |
sofar |
Zeno`: furnaces are not working for me atm... checking server logs |
| 08:06 |
nrzkt |
Tesseract: for #2269 : https://github.com/nerzhul/minetest/commit/cc343292ebd42add30f12b078b0fb7af146ddf87 is correct ? |
| 08:06 |
ShadowBot |
https://github.com/minetest/minetest/issues/2269 -- string.cpp error on android build make |
| 08:08 |
Zeno` |
furnaces? |
| 08:09 |
sofar |
looks like any furnace, grinder, generator isn't working |
| 08:09 |
* sofar |
tries 0.4.11 build |
| 08:10 |
sofar |
no difference, huh |
| 08:10 |
nrzkt |
is http://dev.minetest.net/Android#Minetest right ? or we can use minetest main repo ? |
| 08:12 |
Zeno` |
seems the wiki might be outdated |
| 08:13 |
sofar |
nvm - was 'mod gauges' that broke furnaces |
| 08:13 |
* sofar |
kills it |
| 08:23 |
nrzkt |
hmmmm: i reread irc logs because i forget you suggestion. that's ok for common into src/, i change the PR design |
| 08:23 |
|
ImQ009 joined #minetest-dev |
| 08:30 |
nrzkt |
#2277 rebased. I removed the src/shared like you mentionned hmmmm . Are you find with it ? |
| 08:30 |
ShadowBot |
https://github.com/minetest/minetest/issues/2277 -- Main.cpp cleanup: Move ClientLauncher and InputHandlers to dedicated files by nerzhul |
| 08:31 |
Zeno` |
InputHandlers should never have been in main.cpp in the first place :/ |
| 08:32 |
nrzkt |
ofc, now they are in a good place |
| 08:32 |
nrzkt |
with this change main.cpp ~1000 lines instead of ~2400 lines |
| 08:32 |
nrzkt |
this also permit to remove main #ifdef/#ifndef SERVER |
| 08:32 |
Zeno` |
I agree that the PR is required and useful but don't see the rush for it to be merged. It's unlikely that it will need much rebasing if things wait a bit (main.cpp does not get touched much; when I refactored main.cpp from a 3000 line function it was a PR for over a month and I didn't need to rebase once heh) |
| 08:33 |
nrzkt |
here it's not really a refact, it's only a class move |
| 08:33 |
nrzkt |
refactor* |
| 08:33 |
Zeno` |
Oh, I know... I was just using my experience as an example |
| 08:33 |
Zeno` |
i.e. not many people will change main.cpp and cause your PR to break if you wait a little bit |
| 08:34 |
Zeno` |
I doubt anyone will make a PR that affects main.cpp ;) |
| 08:34 |
Zeno` |
I like it, btw |
| 08:34 |
Zeno` |
So I support it, I just think maybe let the other devs have a say (their timezones seem very different to ours) |
| 08:34 |
nrzkt |
i'll add a comment: merged on sunday if no objection |
| 08:35 |
Zeno` |
Yeah I think waiting a little bit cannot hurt |
| 08:45 |
Zeno` |
#2273 is now running on my server |
| 08:45 |
ShadowBot |
https://github.com/minetest/minetest/issues/2273 -- [Patch 2/4] Network rework: packet writing, sending and cleanups by nerzhul |
| 08:45 |
VanessaE |
and mine. |
| 08:46 |
nrzkt |
#2279 confirmed. If the server is slow and we are doing F7, assert fail, i look at this |
| 08:46 |
ShadowBot |
https://github.com/minetest/minetest/issues/2279 -- Minetest crashes after hitting F7 in GunshipPenguins PvE server |
| 08:49 |
nrzkt |
for this issue, the assert was here: assert(playercao != NULL); (game.cpp L3224) |
| 08:49 |
nrzkt |
why not simply return; if the pointer isn't good. This mean the player isn't loaded yet, then we don't permit to change the camera |
| 08:49 |
nrzkt |
Zeno`, kahrl ? |
| 08:50 |
Zeno` |
hmm |
| 08:51 |
nrzkt |
it's triggered by playerinput, then if the SAO isn't here, we mustn't crash the player. |
| 08:52 |
Zeno` |
should be just a condition rather than an assert then |
| 08:52 |
* Zeno` |
looks |
| 08:52 |
nrzkt |
i compile and test |
| 08:55 |
Zeno` |
so... hmm |
| 08:55 |
Zeno` |
GenericCAO::initialize() has not been called yet? |
| 08:55 |
nrzkt |
no |
| 08:55 |
nrzkt |
because the getCAO return null :) |
| 08:56 |
nrzkt |
it needs more than changing the test |
| 08:56 |
Zeno` |
I don't know enough about the protocol to know how that happens |
| 08:57 |
nrzkt |
okay |
| 08:57 |
nrzkt |
it's fixed |
| 08:58 |
nrzkt |
https://github.com/nerzhul/minetest/commit/a42418ac969d5bb647ec6099dad3a9f8161e8d88 |
| 08:59 |
nrzkt |
we test the player CAO and then change the camera mode, else it permit to change camera mode whereas no playercao |
| 08:59 |
nrzkt |
because current code change the camera mode and crash. |
| 09:00 |
nrzkt |
are you okay with it ? |
| 09:01 |
Zeno` |
yep, go for it |
| 09:01 |
Zeno` |
be back later |
| 09:01 |
celeron55 |
i think it's fine to have one level of subdirectories |
| 09:01 |
celeron55 |
but no more than that |
| 09:01 |
nrzkt |
hi celeron55, okay |
| 09:02 |
celeron55 |
for example in my opinion the organization of the scripting API is kind of annoying because there's a first level that doesn't contain really anything |
| 09:03 |
celeron55 |
(but whatever) |
| 09:06 |
|
crazyR joined #minetest-dev |
| 09:07 |
celeron55 |
all of this really depends on the tools that people edit the code with, though |
| 09:07 |
celeron55 |
for me it's easiest to have no subdirectories |
| 09:08 |
celeron55 |
really ideally there shouldn't be subdirectories but instead files should be tagged |
| 09:08 |
celeron55 |
but our filesystems don't support tagging like that |
| 09:08 |
celeron55 |
nor version control |
| 09:09 |
nrzkt |
the problem is we don't have a good idea which source is client only, which is server only and which is common. Minimum will be to have clientonly and serveronly sources in a subdir, i think |
| 09:10 |
nrzkt |
for new developpers or contributor it's simplier to have a little hierarchy (1 subdir) |
| 09:11 |
celeron55 |
yeah i can see that |
| 09:12 |
celeron55 |
some things are things though that just happen to be used on only the server or only the client while both could use it in the future; such subdirectories add unnecessary work when changes happen to those |
| 09:13 |
celeron55 |
i guess that's not a major issue but the point is, it has downsides |
| 09:14 |
celeron55 |
it seems like client, server and common subdirs would make the most sense |
| 09:14 |
nrzkt |
ofc. server may not have a separated directory because singleplayer need every server api at this time (but maybe server socket listeners are useless and can be added to server instead of common) |
| 09:14 |
celeron55 |
but that's not the only way to make the subdirs |
| 09:14 |
celeron55 |
one way would be to split the code in layers instead |
| 09:14 |
nrzkt |
no, we can have, like now, specialized dirs, network/ lua_api/ ... |
| 09:17 |
celeron55 |
certainly some things should be moved to util/ to begin with |
| 09:18 |
celeron55 |
(that's for self-contained things that don't depend on anything else in the project) |
| 09:27 |
celeron55 |
i tried to figure out a set of broad-stroke subsystems that aren't client or server; it's impossible to have less than like five of them |
| 09:27 |
celeron55 |
five of them more than currently, i mean |
| 09:28 |
celeron55 |
otherwise you're dumping together things that are very much not the same thing |
| 09:28 |
nrzkt |
broad-stroke ? |
| 09:28 |
celeron55 |
i mean like client, server and common are three subdirectories |
| 09:29 |
nrzkt |
oh, i see |
| 09:30 |
|
gregorycu joined #minetest-dev |
| 09:30 |
gregorycu |
How are we all |
| 09:30 |
Zeno` |
common probably isn't necessary (as hmmm inferred) |
| 09:30 |
celeron55 |
trying to split things into three subdirs otherwise, you end up with something like gfx+sfx+gui, the game environment including voxels and mapgen, and then you'd need a place where you can put client and server specific things and various other weird bits and pieces |
| 09:31 |
nrzkt |
i agree with hmmm and Zeno |
| 09:31 |
gregorycu |
Various IDEs have the ability to tag files |
| 09:31 |
gregorycu |
Well, I know visual studio has that |
| 09:31 |
nrzkt |
i'm using a mixup of eclipse, geany and vi |
| 09:32 |
nrzkt |
and this doesn't work :) |
| 09:32 |
celeron55 |
and after that you still need a name for all of those, which is basically impossible 8) |
| 09:33 |
celeron55 |
so the only way to have subdirectories is to have lots of them, which is arguably inconvenient |
| 09:33 |
gregorycu |
Maybe just very general tags would work, server, client, and shared |
| 09:33 |
Zeno` |
I think that stuff in server should be stuff that the client *never* uses |
| 09:33 |
nrzkt |
Zeno` prefers common over shared :) |
| 09:33 |
gregorycu |
Common is fine |
| 09:33 |
Zeno` |
yeah that's just a nomenclature issue hehe |
| 09:33 |
gregorycu |
Shared implies shared object I suppose |
| 09:33 |
gregorycu |
Which is bad |
| 09:33 |
Zeno` |
yeah, that was my concern |
| 09:34 |
Zeno` |
but common is not necessary if both server and client use the file/unit then it can just stay in src/ |
| 09:34 |
celeron55 |
are you aware that that way common ends up containing basically everything that src contains now? |
| 09:35 |
Zeno` |
celeron55, yes. That's why I'm suggesting not having common/ at all |
| 09:35 |
nrzkt |
+1 |
| 09:35 |
gregorycu |
Why do you say that? |
| 09:35 |
Zeno` |
src/ is by "proxy" common |
| 09:36 |
celeron55 |
maybe we could say that everything stays in src unless it is specifically a stable and substantial part which can be cleanly moved to a subdirectory |
| 09:36 |
celeron55 |
in which case it can be moved |
| 09:36 |
Zeno` |
but stuff that is currently surrounded by (e.g.) #ifdef SERVER ... #endif would probably be better in a separate file and put in server/ |
| 09:36 |
celeron55 |
then we don't get any kind of haphazard explosion of subdirs that nobody really agrees on |
| 09:37 |
Zeno` |
oh oh... MKR is on TV |
| 09:37 |
Zeno` |
(sorry wrong channel :/_ |
| 09:37 |
nrzkt |
yes Zeno`, but it's not very simple, because it was used on main.cpp and reduce the code duplication, but sometimes there are large parts of code which could be exported |
| 09:37 |
nrzkt |
xD |
| 09:37 |
Zeno` |
nrzkt, I know |
| 09:38 |
celeron55 |
(altough i'm not sure if that would work either; it might (maybe rightfully so) encourage not changing much) |
| 09:38 |
Zeno` |
I was very aware of the "issue" when I was working on main.cpp :) |
| 09:38 |
Zeno` |
bbiab |
| 09:38 |
|
FR^2 joined #minetest-dev |
| 09:49 |
gregorycu |
celeron55: I found something interesting with our graphics rendering |
| 09:51 |
celeron55 |
finding interesting things in it isn't very hard because it's full of all kinds of weird crap, but what is it? |
| 09:52 |
gregorycu |
On my system, half the render loop is sending shaders the shader constants |
| 09:52 |
celeron55 |
how do you measure that |
| 09:54 |
gregorycu |
profiling using time |
| 09:54 |
gregorycu |
Or sampling |
| 09:54 |
celeron55 |
screenshot? |
| 09:54 |
gregorycu |
One moment, and I'll get one for you |
| 09:55 |
celeron55 |
i find this very hard to believe because the code in minetest doing that is extremely lightweight |
| 09:55 |
celeron55 |
maybe the GPU is some kind of busy state that isn't visible in your profiling |
| 09:55 |
gregorycu |
It's sending the data for every mesh |
| 09:56 |
gregorycu |
1.7 million sends per 6000 frames |
| 09:56 |
gregorycu |
There are 16 shaders |
| 09:56 |
celeron55 |
are you sure it's every mesh, or is it every material switch? |
| 09:56 |
gregorycu |
Probably every material switch |
| 09:57 |
celeron55 |
this is a known problem that everyone is too lazy to solve |
| 09:57 |
celeron55 |
we basically need a texture atlas system |
| 09:57 |
celeron55 |
there was one but it was removed because it broke and everyone was too lazy to fix it |
| 09:58 |
celeron55 |
there's your work cut out for you 8) |
| 09:58 |
gregorycu |
The other work around is to have a callback per shader, and remember when we send the globals and don't send them again |
| 09:58 |
gregorycu |
As they appear to be cached on the GPU |
| 09:58 |
celeron55 |
so is it partly an irrlicht bug? |
| 09:58 |
gregorycu |
Well... |
| 09:58 |
kahrl |
would it help to not send mTransWorld and mInvWorld? they seem to not be used in the shaders |
| 09:59 |
celeron55 |
i would assume that it won't send the same shader multiple times if it doesn't change in the material |
| 09:59 |
gregorycu |
Yes kahrl, it would |
| 09:59 |
gregorycu |
But it's part of the problem |
| 09:59 |
kahrl |
well computing the inverse of a matrix is expensive so it might help a lot |
| 09:59 |
gregorycu |
The docs for irrlicht suggest the shaders set their global once per frame, but it's once per material switch |
| 10:00 |
gregorycu |
Most of the time is spent sending, not on the calculations |
| 10:00 |
gregorycu |
I'm going to shutup for a few min and get a screenshot of my data |
| 10:02 |
celeron55 |
well in any case the core problem is that the material is switched way too often, and the cause of that is that everything uses a different texture, and the cause of that is that there are no texture atlases - i would imagine though that most materials use the same shader and thus the shader parameters don't need to be re-uploaded so that's an optimization issue in irrlricht |
| 10:02 |
celeron55 |
irrlicht* |
| 10:03 |
celeron55 |
but i don't know whether it is the case that the shader doesn't change; if it does, then irrlicht may be doing the right thing |
| 10:03 |
gregorycu |
I believe the stuff is cached per shader |
| 10:03 |
gregorycu |
So we should only send changes when they occur |
| 10:04 |
|
leat joined #minetest-dev |
| 10:04 |
celeron55 |
i believe we have exactly two shaders, one for regular nodes and one for water |
| 10:04 |
celeron55 |
so those cannot switch *that* often if the thing is operating correctly |
| 10:05 |
gregorycu |
hmm... |
| 10:05 |
gregorycu |
I started to look into this, then I backpedled severely |
| 10:05 |
gregorycu |
I thought it was 16 |
| 10:05 |
celeron55 |
the material *does* switch due to texture changes, and there may be things that are rendered without a shader |
| 10:05 |
celeron55 |
in case those can cause something |
| 10:06 |
celeron55 |
wait do we actually have 16 of them |
| 10:07 |
gregorycu |
I believe that to be the case |
| 10:07 |
kahrl |
celeron55: yeah, generate_shader() creates different ones from the same source |
| 10:09 |
kahrl |
by prepending a header containing things like "#define MATERIAL_TYPE ..." |
| 10:09 |
celeron55 |
lol what, it can create like 84 shaders |
| 10:10 |
gregorycu |
I'm a bit out of my depth here to be honest, I don't do graphics |
| 10:10 |
gregorycu |
But I understand profiling |
| 10:12 |
kahrl |
why it creates different shaders for different draw types is beyond me (other than the normal vs. liquid distinction) |
| 10:12 |
kahrl |
the shaders don't even use the DRAW_TYPE constant |
| 10:12 |
celeron55 |
i guess someone might want to optimize it a bit |
| 10:13 |
celeron55 |
also a github issue should be created about this |
| 10:14 |
gregorycu |
http://i.imgur.com/kJTSod9.png |
| 10:14 |
gregorycu |
Second last column is time spent |
| 10:14 |
gregorycu |
In %, including children |
| 10:15 |
gregorycu |
renderMap is 57.37% of the time, updating shaders is 27.49% of the time |
| 10:15 |
gregorycu |
Which means updating shaders is 48% of the time of renderMap |
| 10:18 |
gregorycu |
Does this table make sense to you two? |
| 10:19 |
gregorycu |
https://github.com/minetest/minetest/issues/2283 |
| 10:19 |
gregorycu |
Feel free to pad that with comments and useful things |
| 10:27 |
Zeno` |
it's amazing how different the Linux and Windows profiles look |
| 10:27 |
gregorycu |
I'm all for low hanging fruit |
| 10:27 |
gregorycu |
Would the atlas idea be feasible, because mods "could" add textures at any stage |
| 10:27 |
nrzkt |
what do you think about #2213 ? it seems good |
| 10:27 |
ShadowBot |
https://github.com/minetest/minetest/issues/2213 -- Better Design for Dialogs ( Create World, Delete World, Rename Modpack, ... by fz72 |
| 10:28 |
nrzkt |
What do you think about locking inventory mapping if playerCAO isn't good ? |
| 10:28 |
kahrl |
gregorycu: having atlases for the node tiles would already help a ton, and those are static |
| 10:29 |
nrzkt |
i looked at issue #831 (which is solved now, but not closed) and this opens the inventory, but an empty inventory because it's not loaded. |
| 10:29 |
ShadowBot |
https://github.com/minetest/minetest/issues/831 -- Inventory crash during map load |
| 10:29 |
nrzkt |
if we permit only inventory key when playerCAO is okay, inventory is shown properly |
| 10:29 |
crazyR |
there used to be a texture atlas didnt there? |
| 10:29 |
kahrl |
crazyR: yes |
| 10:30 |
Zeno` |
I can't even see shader updating as part of the callgraph for renderMap() |
| 10:30 |
celeron55 |
Zeno`: it probably depends a lot on the GPU and its driver |
| 10:30 |
crazyR |
kahrl: why was it removed |
| 10:30 |
Zeno` |
celeron55, yeah |
| 10:31 |
celeron55 |
altough, even if you don't see it, it might still be causing performance degradation |
| 10:31 |
kahrl |
crazyR: bugs in several other parts of the engine where for example texture coords weren't set correctly |
| 10:31 |
celeron55 |
if the GPU includes the context switches in the actual rendering call instead |
| 10:31 |
kahrl |
iirc |
| 10:31 |
crazyR |
ahh |
| 10:39 |
|
chchjesus joined #minetest-dev |
| 10:45 |
Zeno` |
I noticed the difference between drivers (even on Linux) when I was looking at the profile Wayward_One prepared |
| 10:45 |
Zeno` |
big differences |
| 10:45 |
Zeno` |
s/profile/profiles |
| 10:51 |
|
Fritigern joined #minetest-dev |
| 10:52 |
Zeno` |
I just wish gregorycu's screenshots showed the headings hehe |
| 10:52 |
Zeno` |
bit hard to comprehend without them, gregorycu :P |
| 10:55 |
Zeno` |
seems I'm talking to myself again |
| 10:56 |
|
chchjesus joined #minetest-dev |
| 10:57 |
nrzkt |
xD |
| 10:58 |
gregorycu |
Who is kwolekr? |
| 11:01 |
|
chchjesus joined #minetest-dev |
| 11:03 |
Zeno` |
That's for me to know and you to find out! :D |
| 11:03 |
Zeno` |
It's a big secret known to only a few people |
| 11:04 |
nrzkt |
it's hmmmm |
| 11:04 |
nrzkt |
he's :) |
| 11:04 |
Zeno` |
oh, go on, ruin it |
| 11:05 |
Zeno` |
gregorycu, do you still have me on ignore? |
| 11:05 |
nrzkt |
i think yes |
| 11:05 |
Zeno` |
lol |
| 11:05 |
|
sofar joined #minetest-dev |
| 11:06 |
|
proller joined #minetest-dev |
| 11:07 |
Zeno` |
Well, I won't bother spending an hour applying, testing, and checking relevant PRs then |
| 11:07 |
Zeno` |
*sigh* |
| 11:09 |
Zeno` |
#2244 seems good even if const MapNode VoxelManipulator::ContentIgnoreNode = MapNode(CONTENT_IGNORE); was the only change |
| 11:09 |
ShadowBot |
https://github.com/minetest/minetest/issues/2244 -- Fix rebase bug, make render loop use cache setting by gregorycu |
| 11:10 |
Zeno` |
I mean it would still be good if that was the only change and no others made |
| 11:11 |
Zeno` |
But I won't spend (up to) hour reading and testing the whole thing. Which is a pity |
| 11:14 |
nrzkt |
Zeno`, celeron55 what do you think about #2284 ? |
| 11:14 |
ShadowBot |
https://github.com/minetest/minetest/issues/2284 -- Open inventory only when inventory could be loaded by nerzhul |
| 11:15 |
nrzkt |
this change doesn't let the user to toggle inventory if it's not loaded. |
| 11:15 |
Zeno` |
nrzkt, why are these issue popping up now? |
| 11:15 |
nrzkt |
which issue ? |
| 11:15 |
|
proller joined #minetest-dev |
| 11:16 |
Zeno` |
the player->CAO() not loaded related ones |
| 11:16 |
nrzkt |
because i looked at it and test some old issues ? |
| 11:16 |
nrzkt |
this fix doesn't prevent a crash, he prevent a bad user experience at connection |
| 11:17 |
Zeno` |
yeah, I'm just wondering why they've never been experienced or reported before. Are they caused by a race condition or something related to optimised code? |
| 11:17 |
nrzkt |
i don't know, the crash pr was done 2 years ago |
| 11:17 |
nrzkt |
now i'll test the same thing with console, bug i think there is the same issue |
| 11:17 |
Zeno` |
2 years? |
| 11:18 |
Zeno` |
I think I must have been looking at the wrong report :D |
| 11:18 |
nrzkt |
maybe a duplicate, no ? |
| 11:18 |
Zeno` |
maybe |
| 11:18 |
nrzkt |
#149 is like issue i fixed in master |
| 11:18 |
ShadowBot |
https://github.com/minetest/minetest/issues/149 -- Segfault when opening console |
| 11:18 |
Zeno` |
But I don't mind that PR; nothing wrong with defensive (and proper) |
| 11:19 |
nrzkt |
i test the crash now, but i think we mustn't open console if playercao not present too |
| 11:19 |
Zeno` |
#2284 looks fine to me |
| 11:19 |
ShadowBot |
https://github.com/minetest/minetest/issues/2284 -- Open inventory only when inventory could be loaded by nerzhul |
| 11:19 |
nrzkt |
okay, i merge it |
| 11:20 |
gregorycu |
What's the deal with 149? |
| 11:21 |
Zeno` |
maybe it's just been overlooked for so long because the_game() used to be so large |
| 11:21 |
Zeno` |
and now things can be narrowed down |
| 11:22 |
Zeno` |
gregorycu, it's probably related to the player->CAO() issue |
| 11:23 |
nrzkt |
it's an old issue, i'll look at it, to see if it works and fix it, or fix the console like 2284 |
| 11:27 |
Zeno` |
Related to mapgen: I don't know if it's because I'm aware of the issue but I'm noticing the shadow issues much more recently |
| 11:28 |
Zeno` |
e.g. shadows on the ground when there is nothing above |
| 11:28 |
Zeno` |
I am supposing it's more related to cavegen |
| 11:28 |
Zeno` |
maybe I'll bisect later and see if it's my imagination or not |
| 11:30 |
Zeno` |
nrzkt, I don't think it's necessary to change commit messages to include "@dev" at the end |
| 11:31 |
nrzkt |
sorry, i like the OpenBSD commits :p |
| 11:31 |
nrzkt |
you receive a mail for each commit ? |
| 11:31 |
Zeno` |
Nah, it just looks funny when browsing history |
| 11:31 |
Zeno` |
I do receive emails already |
| 11:32 |
Zeno` |
If you feel it's necessary maybe add it in the PR? |
| 11:32 |
Zeno` |
e.g. commit 28492849284928492 (@Zeno- approved) or something? |
| 11:33 |
nrzkt |
i don't know |
| 11:34 |
nrzkt |
as you want |
| 11:34 |
Zeno` |
I don't know either; I'd rather see commit messages include only what has changed rather than political issues as well :) |
| 11:35 |
Zeno` |
Maybe the other devs feel differently |
| 11:35 |
Zeno` |
But there is another issue... say I discuss a commit with hmmmm, c55, RBA, kahrl and you... would all be in the @ ? |
| 11:36 |
nrzkt |
stop talking to everybody xD |
| 11:36 |
Zeno` |
I think there must be some trust that devs will commit following the rules/guidelines |
| 11:36 |
Zeno` |
rofl |
| 11:36 |
Zeno` |
ok :D |
| 11:36 |
nrzkt |
i laugh :p |
| 11:38 |
nrzkt |
you are right :) |
| 11:39 |
* Zeno` |
stops talking to everybody |
| 11:40 |
nrzkt |
oh no ! |
| 11:40 |
Zeno` |
lol |
| 11:40 |
Zeno` |
xD |
| 11:40 |
Zeno` |
but I will. Be back later... going onto the Windows to see how the build works :-o |
| 11:40 |
Zeno` |
(yeah I could use a VM) |
| 11:41 |
|
jin_xi joined #minetest-dev |
| 11:59 |
|
oleastre joined #minetest-dev |
| 12:14 |
|
gregorycu_ joined #minetest-dev |
| 13:09 |
|
ElectronLibre joined #minetest-dev |
| 13:13 |
|
PilzAdam joined #minetest-dev |
| 13:15 |
|
selat joined #minetest-dev |
| 13:18 |
|
Player_2 joined #minetest-dev |
| 13:41 |
PilzAdam |
nrzkt, bug! |
| 13:41 |
PilzAdam |
[14:33:28] <CWz> something is very worng with VanessaE's Vanilla minetest server |
| 13:41 |
PilzAdam |
[14:38:47] <PilzAdam> doesn't she run a new network patch? |
| 13:41 |
PilzAdam |
[14:38:51] <PilzAdam> CWz, whats wrong? |
| 13:41 |
PilzAdam |
[14:39:33] <CWz> http://pastebin.com/Zcy6BBXN |
| 13:41 |
PilzAdam |
[14:40:29] <PilzAdam> seems like a network error |
| 13:45 |
|
JakubVanek joined #minetest-dev |
| 13:47 |
Wayward_One |
just tested, I got something similar: http://pastebin.com/xdFN6zTm |
| 13:47 |
Wayward_One |
completely locked up my client |
| 13:48 |
kilbith |
i confirm this, disastrous slowdown and heavily-spammed debug with ERROR[main]: |
| 13:50 |
|
shadowzone joined #minetest-dev |
| 13:53 |
kilbith |
this seems occurs when snowfall mod is installed, others VE servers without this one are OK, and Zeno's server too. |
| 13:54 |
Wayward_One |
yeah, just noticed it on VE-Survival and Creative too |
| 13:54 |
shadowzone |
I have no idea what is going on...so I'm about as confused as confused gets. |
| 14:02 |
|
Zeno` joined #minetest-dev |
| 14:02 |
nrzkt |
what is that ? |
| 14:02 |
nrzkt |
where does this error come from |
| 14:02 |
nrzkt |
sorry for to be late i'm working at work |
| 14:03 |
nrzkt |
snowfall mod you say ? |
| 14:03 |
nrzkt |
when does it come ? at start ? can you be more precise |
| 14:03 |
Wayward_One |
for me, almost as soon as i join the servers |
| 14:03 |
shadowzone |
Hello, Zeno`. |
| 14:03 |
nrzkt |
it's a problem on client side ? |
| 14:03 |
Zeno` |
hello |
| 14:05 |
Wayward_One |
not sure if it's also server-side, all i know is it floods my chat and console and locks up my client |
| 14:06 |
nrzkt |
ok, strange i haven't seen this issue before, but i haven't tried with this mod |
| 14:06 |
nrzkt |
what client are you using ? 0.4.11 stable ? master ? |
| 14:07 |
Wayward_One |
tried with both latest git and a month-old build |
| 14:08 |
|
CWz joined #minetest-dev |
| 14:11 |
Zeno` |
save me looking at the logs. What is the problem? |
| 14:12 |
Wayward_One |
http://pastebin.com/xdFN6zTm |
| 14:12 |
Zeno` |
does it happen if you connect to my server? |
| 14:13 |
kilbith |
no |
| 14:13 |
Wayward_One |
upon joining VE-Vanilla, Creative, and Survival, chat and console get flooded with that |
| 14:13 |
kilbith |
where 'snowfall' is installed -> bug |
| 14:13 |
nrzkt |
hmmm... let me a minute |
| 14:14 |
Zeno` |
ridiculous error message in the first place, heh |
| 14:15 |
Zeno` |
ERROR[main] .... thanks for nothing silly client |
| 14:15 |
Wayward_One |
lol |
| 14:16 |
nrzkt |
hmmm |
| 14:16 |
nrzkt |
the bug is not network |
| 14:16 |
nrzkt |
i think, wait a second |
| 14:16 |
nrzkt |
very difficult to catch in my window :p |
| 14:18 |
nrzkt |
same issue, but i saw a problem with CURL |
| 14:20 |
nrzkt |
server has problem to download index.mts file, i saw a 405 |
| 14:22 |
nrzkt |
wait a minute, i test 0.4.12 frozen version with this mod |
| 14:23 |
nrzkt |
and next i install it on a local server with 0.4.12-freeze, and network part 1 and next network part 2 |
| 14:25 |
|
proller joined #minetest-dev |
| 14:32 |
shadowzone |
Zeno`, CWz is killing your server. |
| 14:32 |
shadowzone |
XD |
| 14:33 |
nrzkt |
at this time: 0.4.12 -> 0.4.12: no problem. 0.4.12 -> master: no pb. master -> master: no pb |
| 14:33 |
nrzkt |
now i test with patch |
| 14:41 |
nrzkt |
right there is a bug with the particles or other thing |
| 14:43 |
nrzkt |
network 2/4 client also have the problem |
| 14:44 |
|
Zeno` joined #minetest-dev |
| 14:57 |
|
MinetestForFun joined #minetest-dev |
| 14:58 |
|
CraigyDavi joined #minetest-dev |
| 15:06 |
nrzkt |
ok |
| 15:06 |
nrzkt |
i have found an interesting thing |
| 15:06 |
nrzkt |
it seems the texture loading error is on packet 0x46, because i have 7 0x46 packets receive and 7 image error. |
| 15:07 |
nrzkt |
but before fixing the packet bug, i look at the ugly output on the console |
| 15:07 |
|
Wayward_One joined #minetest-dev |
| 15:08 |
nrzkt |
oh... it uses a strange deserialization mode, i look at the last PR (network P4) maybe it's properly fixed |
| 15:13 |
nrzkt |
hmmm with current master the reading is done with same way than master, i look at server packet sending |
| 15:17 |
|
hmmmm joined #minetest-dev |
| 15:18 |
nrzkt |
oh |
| 15:18 |
nrzkt |
i found the issue.... a duplicate insert into the packet into server side... |
| 15:18 |
nrzkt |
*pkt << pos << velocity << acceleration << expirationtime |
| 15:18 |
nrzkt |
<< expirationtime << size << collisiondetection; |
| 15:18 |
nrzkt |
pkt->putLongString(texture); |
| 15:18 |
nrzkt |
*pkt << vertical; |
| 15:18 |
nrzkt |
double expiration time... |
| 15:19 |
nrzkt |
i test the fix and commit it |
| 15:21 |
|
Fritigern joined #minetest-dev |
| 15:22 |
Zeno` |
nrzkt, what patch # (of yours) was I testing? |
| 15:23 |
nrzkt |
same as vanessaE |
| 15:23 |
Zeno` |
yeah, I can't find it |
| 15:23 |
Zeno` |
anyway there is something wrong |
| 15:23 |
Zeno` |
with it applied client's can crash the server |
| 15:24 |
Zeno` |
ask CWz for more info |
| 15:24 |
Zeno` |
I've removed the patch from my server for now |
| 15:24 |
nrzkt |
okay. |
| 15:24 |
Zeno` |
Wayward_One knows how to reproduce as well I think |
| 15:25 |
nrzkt |
no problem , we must find all the issues. TOCLIENT_SPAWN_PARTICLE now works properly, i commit & rebase |
| 15:25 |
nrzkt |
do you have the crashlog ? |
| 15:25 |
nrzkt |
or the assert, or something other ? |
| 15:26 |
Zeno` |
I don't |
| 15:26 |
Zeno` |
it's not as simple as that |
| 15:26 |
nrzkt |
CWz do you have informations ? |
| 15:27 |
|
selat joined #minetest-dev |
| 15:27 |
Zeno` |
nrzkt, one sec |
| 15:28 |
Zeno` |
ok, sending PM |
| 15:38 |
|
JakubVanek joined #minetest-dev |
| 15:39 |
|
nore joined #minetest-dev |
| 15:40 |
Zeno` |
nore, please allow ShadowNinja to approve and merge PRs :( |
| 15:40 |
nore |
Zeno`, I thought I did that a few days ago... |
| 15:40 |
Zeno` |
ok |
| 15:41 |
Zeno` |
does he know? :) |
| 15:41 |
kilbith |
Zeno`: http://dev.minetest.net/minetest_game_Development |
| 15:41 |
nore |
well, I sent the message to ShadowNinja since I didn't know that ShadowNinja == Tesseract |
| 15:41 |
nore |
so I'm not sure he knows |
| 15:42 |
celeron55 |
nrzkt's network changes are really unsafe |
| 15:42 |
celeron55 |
they don't do proper bounds checking when reading the data array |
| 15:42 |
Zeno` |
celeron55, I've notified him of that |
| 15:43 |
celeron55 |
most of what we did previous was using ostringstream (not all), which was always safe |
| 15:43 |
Zeno` |
and also removed the patch from my server |
| 15:43 |
celeron55 |
previously* |
| 15:43 |
celeron55 |
i mean, istringstrea |
| 15:43 |
nrzkt |
celeron55 i am fixing a use after free |
| 15:43 |
celeron55 |
+m |
| 15:44 |
nrzkt |
celeron55 can you give me an example of missing bound checking, i'll fix it |
| 15:44 |
celeron55 |
i'm not going to list them all |
| 15:44 |
nrzkt |
give me only one example and i fix all |
| 15:45 |
celeron55 |
https://github.com/minetest/minetest/blob/master/src/network/networkpacket.cpp#L185 |
| 15:45 |
celeron55 |
the check is for 1-byte data while it reads 8 |
| 15:45 |
celeron55 |
also it shouldn't use assert |
| 15:46 |
celeron55 |
it probably should throw a SerializationError or whatever it was called |
| 15:46 |
nrzkt |
https://github.com/nerzhul/minetest/blob/packet-handling_2/src/network/networkpacket.cpp |
| 15:47 |
nrzkt |
please look at this, it's patch 2/4 |
| 15:47 |
celeron55 |
it has the same problem |
| 15:47 |
nrzkt |
you are right, sorry, i'm tired |
| 15:48 |
|
FR^2 joined #minetest-dev |
| 15:48 |
hmmmm |
for what it's worth, I am not a fan at all of operator overloading |
| 15:48 |
hmmmm |
if you're writing a u16 to a packet, why hide that... |
| 15:48 |
hmmmm |
i think that's very noteworthy information |
| 15:49 |
celeron55 |
well that overloading is in the bounds of sanity, because it's streaming data into the packet just like we previously used std::i/ostringstream |
| 15:49 |
celeron55 |
altough, hiding the data type is a very valid problem |
| 15:49 |
nrzkt |
we can show it by adding the type when we stream it |
| 15:49 |
celeron55 |
it may cause unwanted protocol breakage when internal types change |
| 15:50 |
nrzkt |
i can prefix each insert |
| 15:50 |
celeron55 |
is it possible to make an explicit template parameter mandatory in C++? |
| 15:50 |
celeron55 |
that would be a good way here |
| 15:51 |
nrzkt |
do you want a thing like pkt->insert<u8>(data); ? |
| 15:51 |
hmmmm |
are templates actually necessary for this |
| 15:51 |
celeron55 |
yes i mean allowing to leave the <u8> out is a bad thing when formatting and reading packets |
| 15:52 |
celeron55 |
unless the packet contain type information, but we don't want that |
| 15:52 |
celeron55 |
or, well, we can decide to want it, but i think many would think it's too bloated |
| 15:52 |
hmmmm |
we shouldn't be using (bad) C++ features like that for little to no reason when there is a non-C++y alternative that works even better |
| 15:52 |
celeron55 |
(i've suggested it previously) |
| 15:54 |
celeron55 |
C++ has many well-intended features that aren't applicable to a lot of cases due to practical reasons |
| 15:54 |
hmmmm |
C++ is simply a bad language |
| 15:54 |
hmmmm |
i can't wait until we get something better |
| 15:55 |
celeron55 |
well... rust 1.0 is out soon, it's the closest to sanity out of what's out there now |
| 15:55 |
Zeno` |
c'mon hmmmm... they're adding lots of cool stuff like Cairo |
| 15:55 |
celeron55 |
8) |
| 15:55 |
nrzkt |
then celeron55 i'll study a solution to have the right reading/writing value, but a useless cast in streaming interface could be good, no ? |
| 15:56 |
hmmmm |
heh |
| 15:56 |
celeron55 |
what do you mean by "a useless cast in streaming interface" |
| 15:56 |
hmmmm |
he means like |
| 15:56 |
nrzkt |
*pkt << (u8) value; |
| 15:56 |
hmmmm |
yeah |
| 15:56 |
hmmmm |
it's useless only in most cases until you accidentally use the wrong datatype |
| 15:57 |
celeron55 |
what i want is something where breaking the protocol is maximally difficult to do |
| 15:57 |
celeron55 |
automatic type inference does not accomplish that |
| 15:57 |
celeron55 |
it makes it easier to break |
| 15:58 |
nrzkt |
0.4.12-freeze permit to break the protocol too |
| 15:58 |
celeron55 |
it can be broken only once; after it, it should be solid as a rock so that people are unlikely to break it accidentally |
| 15:59 |
nrzkt |
what do you suggest ? |
| 15:59 |
Zeno` |
backpedal... the issue I have is that two users worked out how to crash my server |
| 15:59 |
Zeno` |
in less than 5 hours and they didn't even know I had a patch applied |
| 16:00 |
celeron55 |
well as a fact, if you don't use templates in the packet-reading and writing interface, there is no automatic typing of packet data, which solves that problem (then you have to explicitly change the method name to change what goes in and out of the packet) |
| 16:00 |
celeron55 |
of course it isn't using so much C++ features then, but that's not the goal anyway |
| 16:00 |
nrzkt |
no but we loose elegant working code |
| 16:01 |
celeron55 |
elegance is not the main concern when packing and unpacking packets |
| 16:01 |
celeron55 |
the main concern is making correct packets |
| 16:02 |
celeron55 |
and reading them correctly |
| 16:02 |
nrzkt |
the current interface permit it, but we are not catching the error, we assert the receiver/transmitter, which doesn't permit protocol modifications |
| 16:03 |
celeron55 |
something like google's protocol buffers is one elegant-ish way that also makes it easy to keep the protocol from breaking |
| 16:03 |
nrzkt |
do you have source ? it would be interesting |
| 16:03 |
celeron55 |
https://code.google.com/p/protobuf/ |
| 16:03 |
celeron55 |
it has a separate packet description file which determines what packets are allowed to be made |
| 16:04 |
celeron55 |
there are other similar implementations out there and rolling our own isn't impossible either |
| 16:05 |
celeron55 |
really the PR that you closed that had some stuff from me and sfan5 was one way to roll our own (an incomplete one though) |
| 16:06 |
celeron55 |
anyway my main point is: the protocol has been too easy to accidentally break previously; we need something that is harder to accidentally break |
| 16:07 |
|
SopaXorzTaker joined #minetest-dev |
| 16:07 |
nrzkt |
then you meen we must have a struct for each packet type |
| 16:07 |
nrzkt |
mean* |
| 16:07 |
celeron55 |
in the protobuf style, there is only one canonical definition which determines the packet's format |
| 16:07 |
|
SopaXorzTaker joined #minetest-dev |
| 16:07 |
celeron55 |
which is kind of a struct |
| 16:08 |
celeron55 |
and then there are checks that make it so that the sender cannot make anything different, and the receiver will not accept anything different |
| 16:09 |
nrzkt |
it's perfect |
| 16:09 |
nrzkt |
then i can use it if you prefer it. |
| 16:10 |
celeron55 |
i'd like to hear if someone else has thoughts or experiences on this |
| 16:10 |
hmmmm |
oh god no please don't use protobuf |
| 16:11 |
hmmmm |
say 'no' to generated code spew |
| 16:11 |
hmmmm |
say 'no' to bloated interfaces |
| 16:11 |
|
roniz joined #minetest-dev |
| 16:11 |
hmmmm |
serializing data isn't a difficult problem, and it's one that's already been solved here |
| 16:12 |
hmmmm |
why are you adding more SHIT for no reason |
| 16:12 |
hmmmm |
all you need to do is create a good interface |
| 16:12 |
celeron55 |
it's the KISS way for sure |
| 16:13 |
nrzkt |
i agree with hmmmm, but i understand celeron55 |
| 16:13 |
celeron55 |
and not using templates allows it to be more resistant to breaking |
| 16:13 |
hmmmm |
we use protobuf at work and it's painful for sure |
| 16:13 |
celeron55 |
protobuf doesn't have any mechanisms for versioning, right? |
| 16:14 |
hmmmm |
it doesn't |
| 16:14 |
nrzkt |
Zeno` could you use #2273, i fixed the use after free (and also the particle packet) |
| 16:14 |
ShadowBot |
https://github.com/minetest/minetest/issues/2273 -- [Patch 2/4] Network rework: packet writing, sending and cleanups by nerzhul |
| 16:14 |
celeron55 |
versioning is one thing that's a bit of a mess in MT, so that's one goal |
| 16:15 |
celeron55 |
i think the obvious way to go for now is pkt->insertU8(data); |
| 16:15 |
nrzkt |
ouch :( |
| 16:15 |
celeron55 |
that allows cleaner code but does not cause the extra hazards of templates |
| 16:16 |
nrzkt |
what is the difference between pkt->insert<u8>() and insertu8 ? |
| 16:17 |
celeron55 |
templates would be fine if the type inference could be disabled, but it looks like it cannot be |
| 16:17 |
celeron55 |
^ |
| 16:17 |
hmmmm |
they also make compilation slower |
| 16:17 |
celeron55 |
the compilation overhead isn't that much in a simple case like that |
| 16:18 |
hmmmm |
i agree this is the kind of stuff that templates were made for |
| 16:18 |
hmmmm |
but there are still problems |
| 16:19 |
celeron55 |
it's sad but also the reality |
| 16:20 |
celeron55 |
i love it how people come from programming schools knowing all this template stuff, and then they can't ever actually use it because there are practical problems with it 8)) |
| 16:22 |
nrzkt |
then the problem here is we hide the insert type, right ? |
| 16:24 |
nrzkt |
i can change the NetworkPacket API to show it |
| 16:25 |
celeron55 |
how would you do that |
| 16:25 |
nrzkt |
if you don't like my streaming operator |
| 16:25 |
nrzkt |
we could use a ugly and repertitive insert |
| 16:25 |
nrzkt |
of we could add a cast |
| 16:26 |
|
crazyR joined #minetest-dev |
| 16:27 |
|
Guest6503 joined #minetest-dev |
| 16:27 |
celeron55 |
this wouldn't be a problem if we had a test system with proper cross-version testing with complete network packet coverage |
| 16:27 |
celeron55 |
but... that's very far fetched |
| 16:27 |
celeron55 |
never going to happen |
| 16:27 |
nrzkt |
and this doesn't prevent hacking clients which tried to crash servers, like a bug i fixed 1 month ago |
| 16:28 |
nrzkt |
i thing the API can be improved |
| 16:32 |
Zeno` |
the currently discussed crash has nothing to do with hacking |
| 16:33 |
Zeno` |
even if a client was hacked it should not be able to crash a server |
| 16:34 |
nrzkt |
i agree with you, and i must improve the NetworkPacket to delete this issue. |
| 16:34 |
nrzkt |
but i think try catch is the only path :s |
| 16:35 |
Zeno` |
that's unfortunate :( I don't like exceptions much at all |
| 16:35 |
nrzkt |
i agree but it's a special layer which needs it i think, we don't have choice |
| 16:35 |
celeron55 |
what, that's exactly the kind of situations where exceptions do make sense |
| 16:35 |
celeron55 |
if you never use exceptions elsewhere, that's where you *do* want to use them |
| 16:35 |
nrzkt |
then i'll replace all asserts with throw |
| 16:36 |
Zeno` |
celeron55, it cannot be solved without using exceptions? |
| 16:36 |
nrzkt |
not with elegant code, adding a boolean could present using a throw, but it's very very fat |
| 16:37 |
|
Krock joined #minetest-dev |
| 16:38 |
Zeno` |
I disagree (and so do a lot of other people), but anyway |
| 16:38 |
|
ImQ009 joined #minetest-dev |
| 16:38 |
Zeno` |
I noticed a reference to some Google code earlier. Google pretty much forbid their programmers using exceptions |
| 16:40 |
celeron55 |
guys |
| 16:40 |
celeron55 |
we have used exceptions to succesfully handle these errors previously |
| 16:40 |
celeron55 |
we can do it in the future too |
| 16:40 |
celeron55 |
exceptions have been a problem elsewhere but not here |
| 16:40 |
celeron55 |
just don't even start messing with it; it's unproductive |
| 16:41 |
|
zat joined #minetest-dev |
| 16:42 |
Zeno` |
I'm not going to mess with it; I'm just saying that elegant code does not *require* exceptions |
| 16:42 |
Zeno` |
and that many people agree. *shrug* |
| 16:43 |
celeron55 |
i asked #elsewhere about disabling the type inference |
| 16:43 |
celeron55 |
looks like it can be done |
| 16:44 |
celeron55 |
template<typename U, typename T> void insert(T t, std::enable_if<std::is_same<T, U>::value>::type* = nullptr) |
| 16:44 |
celeron55 |
you do something like this |
| 16:44 |
celeron55 |
U is given explicitly, T is inferred, and then there's just a compile time check |
| 16:46 |
nrzkt |
if types are different, what happens ? |
| 16:46 |
celeron55 |
compilation fails if the inferred type is different than the first (only) supplied template parameter |
| 16:47 |
nrzkt |
then, how we call the function in reality ? |
| 16:47 |
nrzkt |
pkt->insert<u8>(toto) ? |
| 16:47 |
celeron55 |
yeah, it's just that |
| 16:47 |
nrzkt |
it could be good then |
| 16:47 |
celeron55 |
(i didn't test that code; but it's something close to what would work) |
| 16:50 |
nrzkt |
it could be a good improvement for code |
| 16:50 |
nrzkt |
i thought about it. |
| 16:54 |
|
Calinou joined #minetest-dev |
| 16:55 |
nrzkt |
i must go. celeron55 i replace asserts by throw SerializationError like you suggest to prevent special crafted packet which could crash server. |
| 16:57 |
nrzkt |
i will improve the throw test this evening or tomorrow. For your template idea i will think about it in the future but i think i will not merge it for PR2. Maybe an intermediate between PR2 and PR3 (PR3 repairs the protocol and change some bad handled packets, and unify some APIs) |
| 16:57 |
celeron55 |
looks like here's another way of doing it http://paste.dy.fi/2BS |
| 16:57 |
nrzkt |
for packet writing it's dynamic |
| 16:58 |
celeron55 |
that is probably better because it doesn't compare types but instead just requires a parameter |
| 16:58 |
celeron55 |
i.e. it allows making an implicit cast |
| 16:58 |
nrzkt |
it cannot be used, because we have the endianess problem after |
| 16:59 |
nrzkt |
it's difficult to handle write and read with template... impossible in fact, because of endianess |
| 16:59 |
nrzkt |
some types needs special handling. I must go, see you later |
| 17:01 |
celeron55 |
no that's not a problem because the underlying functions can support endianess directly |
| 17:02 |
celeron55 |
you just have to avoid copying memory as-is and instead create template implementations for the primitive types |
| 17:03 |
|
shadowzone joined #minetest-dev |
| 17:04 |
|
est31 joined #minetest-dev |
| 17:05 |
|
kilbith joined #minetest-dev |
| 17:06 |
|
zat joined #minetest-dev |
| 17:08 |
|
ImQ009 joined #minetest-dev |
| 17:19 |
|
CWz left #minetest-dev |
| 17:25 |
|
nore joined #minetest-dev |
| 17:29 |
|
Hunterz joined #minetest-dev |
| 17:53 |
|
shadowzone joined #minetest-dev |
| 17:58 |
|
proller joined #minetest-dev |
| 17:59 |
|
aaaaa joined #minetest-dev |
| 18:09 |
|
ElectronLibre joined #minetest-dev |
| 18:18 |
|
nrzkt joined #minetest-dev |
| 18:22 |
nrzkt |
#2286 seems to be good. Nobody have problem with it ? |
| 18:22 |
ShadowBot |
https://github.com/minetest/minetest/issues/2286 -- Fix Download complete dialog in the mods store by ngosang |
| 18:28 |
|
alexxs joined #minetest-dev |
| 18:39 |
|
kilbith joined #minetest-dev |
| 18:42 |
MattJ |
Is there any particular reason that most mods supply their own (and all different!) config system (config.txt, settings.txt, config.lua, etc....) rather than using the core settings API? |
| 18:46 |
nrzkt |
the liberty ? |
| 18:47 |
MattJ |
If that's the only reason, ok :) |
| 18:47 |
MattJ |
I wondered if there was some non-obvious reason that I couldn't/shouldn't use it in my own mods |
| 18:48 |
nrzkt |
i think, yes. But maybe everybody doesn't know about it |
| 18:51 |
|
shadowzone joined #minetest-dev |
| 18:55 |
celeron55 |
well, i'd imagine if you have hundred mods and all of them would use the centralized settings, it becomes bit of a mess |
| 18:56 |
celeron55 |
but dunno, i mean a config.lua is effectively a non-cost solution for mods as they have to have the magic values somewhere in any case and that's just a source file |
| 18:56 |
celeron55 |
.txt files are weird as they're way more work than either of those other solutions |
| 18:57 |
nrzkt |
what about #2227 ? (non core-dev people can talk for design changed) |
| 18:57 |
ShadowBot |
https://github.com/minetest/minetest/issues/2227 -- Small changes in the style of controls by ngosang |
| 18:57 |
nrzkt |
changes* |
| 19:01 |
nrzkt |
celeron55 can i get the right status on forum.minetest.net please ? :) |
| 19:03 |
Calinou |
do we have a standard config API? |
| 19:06 |
fireglow |
use https://github.com/vstakhov/libucl |
| 19:08 |
nrzkt |
lua/build/liblua.a(loslib.o): dans la fonction « os_tmpname »: |
| 19:08 |
nrzkt |
everybody has this ? |
| 19:09 |
Calinou |
use LANG=C for English messages :P |
| 19:09 |
Calinou |
yes I do |
| 19:09 |
nrzkt |
the use of `tmpnam' is dangerous, better use `mkstemp' |
| 19:09 |
est31 |
yes /me too ! |
| 19:09 |
nrzkt |
i'll look at it when i get a little bit more ime |
| 19:12 |
est31 |
I can look at it too |
| 19:12 |
* est31 |
looks at it |
| 19:15 |
celeron55 |
mkstemp is posix, thus lua won't use it |
| 19:16 |
celeron55 |
tmpnam is in the C standard so that's what it will use |
| 19:16 |
celeron55 |
it's not our concern |
| 19:19 |
est31 |
man tmpnam sais Note: Avoid use of tmpnam(); use mkstemp(3) or tmpfile(3) instead. |
| 19:19 |
est31 |
tmpfile is bare c |
| 19:21 |
celeron55 |
umm wait... "By default, Lua uses tmpnam except when POSIX is available, where it uses mkstemp" |
| 19:22 |
celeron55 |
this means our lua cmake stuff is missing the setting of LUA_USE_MKSTEMP when posix is supported |
| 19:22 |
est31 |
yes |
| 19:22 |
celeron55 |
should be easy to add |
| 19:23 |
nrzkt |
it's just adding a variable for POSIX ? |
| 19:24 |
nrzkt |
then in every supported UNIX but not windows, right ? |
| 19:24 |
celeron55 |
yeah i guess something like if(POSIX) add_definition(whatever) endif() |
| 19:25 |
celeron55 |
regular lua uses an autotools based system and ours is a rather minimalist cmake setup so it's not surprising that it's missing a thing or two |
| 19:25 |
est31 |
there is already a if (DEFAULT_POSIX) there |
| 19:27 |
celeron55 |
it's missing the setting of LUA_USE_POSIX if it detects being on posix |
| 19:27 |
celeron55 |
it just sets some DEFAULT_POSIX cmake variable... with which it does nothing while there is an if() for it 8) |
| 19:28 |
est31 |
there is LUA_USE_{LINUX, MACOSX} |
| 19:28 |
est31 |
rather set that than DEFAULT_POSIX |
| 19:28 |
celeron55 |
there are no add_definitions() calls in there so nothing will end up in C |
| 19:29 |
celeron55 |
and no cmake-generated configuration files either |
| 19:30 |
nrzkt |
if(DEFAULT_POSIX) |
| 19:30 |
nrzkt |
option(LUA_USE_POSIX ON) |
| 19:30 |
nrzkt |
else() |
| 19:30 |
nrzkt |
endif() doesn't work :) |
| 19:30 |
est31 |
perhaps s/ON/"" ON/ ? |
| 19:30 |
est31 |
or better, s/option/set& |
| 19:31 |
nrzkt |
doesn't work |
| 19:31 |
est31 |
gonna try another method... |
| 19:31 |
nrzkt |
i must go away. If you solve the issue please PR |
| 19:32 |
est31 |
ok :) |
| 19:32 |
|
JakubVanek joined #minetest-dev |
| 19:38 |
|
shadowzone joined #minetest-dev |
| 19:38 |
|
JakubVanek joined #minetest-dev |
| 19:44 |
|
rubenwardy joined #minetest-dev |
| 19:46 |
|
Miner_48er joined #minetest-dev |
| 19:54 |
est31 |
you have to actually pass that config value |
| 19:54 |
est31 |
I'll do a PR |
| 20:07 |
est31 |
pr is out #2290 |
| 20:07 |
ShadowBot |
https://github.com/minetest/minetest/issues/2290 -- Fix two gcc warnings for lua by est31 |
| 20:09 |
|
nrzkt joined #minetest-dev |
| 20:13 |
est31 |
pr is out nrzkt |
| 20:26 |
|
Selah joined #minetest-dev |
| 21:04 |
nrzkt |
thanks est31, i finish my wow raid and i look at this, or tomorrow :) |
| 21:08 |
est31 |
ok |
| 21:12 |
|
shadowzone joined #minetest-dev |
| 21:38 |
|
ElectronLibre left #minetest-dev |
| 21:47 |
|
Wayward_One_ joined #minetest-dev |
| 21:47 |
|
Selah joined #minetest-dev |
| 21:56 |
|
proller joined #minetest-dev |
| 22:07 |
|
shadowzone joined #minetest-dev |
| 22:14 |
|
Player_2 joined #minetest-dev |
| 22:37 |
est31 |
what do you think, what is better 1) change bundled lua code (only a comment change), 2) disable Wall warnings for bundled lua or 3) live with the warning |
| 22:37 |
est31 |
I'm talking bout the *.c change in #2290 |
| 22:37 |
ShadowBot |
https://github.com/minetest/minetest/issues/2290 -- Fix two gcc warnings for lua by est31 |
| 22:43 |
fireglow |
est31: dang I forgot: Really big thank you for fixing Pipeworks! :) |
| 22:43 |
fireglow |
you are our servers saviour, again :) |
| 22:44 |
est31 |
lol great :) |
| 22:46 |
fireglow |
commented on the pull-req |
| 23:08 |
|
Fritigern joined #minetest-dev |
| 23:13 |
|
Player_2 joined #minetest-dev |
| 23:17 |
|
shadowzone joined #minetest-dev |
| 23:55 |
|
Taoki joined #minetest-dev |
