Time Nick Message 15:26 Wuzzy Is using table.copy a big performance issue and should it be avoided in builtin? 15:27 Wuzzy for example, is it OK to use table.copy to deep-copy a node table? 15:27 Wuzzy or should i copy it "by hand" with {name="foo:bar", param1=0, param2=45}? 15:27 sfan5 I think by hand is preferable there 15:27 Wuzzy why? 15:27 Wuzzy can we trust node tables to stay the same forever? 15:30 celeron55_ they're part of the api so... yes? 15:31 celeron55_ maybe there should be a function for making a copy of a node table though 15:31 Wuzzy ok thanks 15:32 Wuzzy eh, i dont think such a functtion is that important, i was just askin bout conventions ? 15:35 MTDiscord The easiest way to ensure that param3 eventually gets added to the node tables is to hard-code the table copy all over the place to ensure that it would be maximally inconvenient to add support for it... 15:40 celeron55_ this all of course depends on details that were not explained 15:40 celeron55_ but in builtin a simple grep will give us every line that needs to be changed if the engine is changed in some way, that's not a problem in any case 15:42 celeron55_ mods might end up with some compatibility problems, that's for sure 15:43 celeron55_ so if you were asking that exact question for a mod that deals with copied node tables, you may have reasons to do it differently 16:46 Wuzzy Reminder to all core devs: builtin is translatable now, remember to enclose your player-facing server messages in S() 16:49 nerzhul nobody cares about my proposal about a irrlicht header retribution to the lib ? :( 17:05 MTDiscord We should abolish the entire copy-thing IMO. 17:08 MTDiscord For compatibility reasons we can't do that for old callbacks, but we should do it for newly added ones. 17:14 pgimeno FWIW I don't think it matters whether a deepcopy is inlined or not, performance wise. The copy itself of the nodes table is expected to make the function call overhead negligible. 17:18 MTDiscord I'm saying we should deprecate call-by-value yesterday and have all new callbacks not do it anymore 17:21 pgimeno I may be misunderstanding the use case 17:22 pgimeno certainly, avoiding pairs() has potential for speeding up things because it prevents JIT compilation, so if the alternative does not involve pairs(), it may be better 17:55 rubenwardy nerzhul: I can't see any such proposal 18:07 nerzhul 1 PR on each repo and talked about it this weekend 18:08 nerzhul rubenwardy ^ 21:33 v-rob I had another (not fully developed) idea about extending node possibilities beyond param2, and I want to run it by some other people to see any problems with it 21:34 v-rob The Minetest world, in general, is fairly homogenous. Most of the world is air, stone, or things that are identical to each other 21:35 v-rob So, instead of storing MapNodes in MapBlock, store pointers to MapNodes and put all the MapNodes in a separate list. Memory usage would increase initially since pointers are at least the same size as MapNodes, but later it might be more efficient memory-wise if more is added to MapNode 21:36 v-rob Only one MapNode would need to be stored for all air nodes, stone nodes, etc 21:37 DS-minetest isn't this basically the content id? 21:37 sfan5 it's another lay of indirection on top 21:37 sfan5 layer* 21:37 v-rob Right, but it also includes param2 21:37 v-rob I don't know if performance would suffer or not 21:38 DS-minetest ah. so, would the MapBlock lists be stored per block or per whole world? 21:38 celeron55_ you're going to have to imagine and write down quite a list of benefits for a system like that to be created 21:38 celeron55_ it would be a *lot* slower 21:39 celeron55_ as in, quite fast, but not blazing fast like the current simple system 21:39 celeron55_ 128 bit nodes would be faster than that 21:39 v-rob Yes, that's why I'm running it past everyone first :) 21:39 celeron55_ maybe even 256 21:40 celeron55_ or larger 21:40 v-rob However, memory usage would be quite smaller than 256 MapNodes, though 21:41 v-rob 128... maybe not 21:41 celeron55_ what are the usecases for larger nodes that fit between current nodes and node metadata 21:41 v-rob I guess it depends on whether the world is mostly homogenous or not 21:41 celeron55_ and how much data they need 21:41 celeron55_ that's the question 21:42 v-rob Isn't node metadata too slow to be used in rendering? 21:43 celeron55_ no? 21:43 v-rob Thought it was, but I don't think anyone would consider storing param2 in metadata for every node 21:44 celeron55_ well what you just proposed was basically more tightly packed node metadata 21:44 celeron55_ it would have the same characteristics, just somewhat smaller 21:44 v-rob But would be quite a bit cheaper memory-wise than metadata 21:44 celeron55_ that is, still can't bloat up every node with extra stuff 21:45 DS-minetest btw. why aren't param0, param1 and param2 each stored separately? this would make it easier to allow special (but very common) mapblocks with 8 bit param0; and with SIMD operations one could more easily find nodes of a specific id faster; isn't it pretty common to only look at the content id for most nodes? 21:46 celeron55_ DS-minetest: it's also very common to fetch many or all of them for a single node 21:47 celeron55_ i guess the main reason is because this way allows a normal class structure for nodes 21:48 v-rob Variable-length structures in an array are difficult 21:48 DS-minetest well yes, but isn't that mostly because of the lua api that only has the get_node and no get_nodename or similar? 21:48 celeron55_ if you do separate arrays in C++, the optimization ends up messing up code readability really bad 21:49 v-rob An adaption on my idea: Store param2 in metadata, but in an optimized form. So, minetest.get_meta():set_param(value), implemented without string-based lookup internally for RAM and performance benefits 21:50 v-rob Also frees up former param2 space in MapNode for e.g. param1 to extend to colored lighting 21:51 v-rob Yeah, that idea sounds better than the original one, but maybe not perfect still 21:51 celeron55_ i don't recommend wasting the time to make that just to benchmark it being bad 21:51 celeron55_ is the most polite way for me to put this 21:52 sfan5 you have to consider that mesh making right now can avoid touching metadata at all 21:52 sfan5 bringing metadata into it will make it slow 21:52 v-rob Don't worry about politeness, I want my ideas to be torn apart if they're bad 21:53 celeron55_ there are so many ways to "improve" the node storage, all of them being bad, and i don't want to explain the badness of each individually 21:54 celeron55_ anyway i'll let you discuss 8) of course there can be one good one among a hundred bad ones that i'm not willing to look into 21:55 celeron55_ i'd like to underline what i said previously: 21:55 celeron55_ if there's a need for more fast bulk data per node, we, or i, will figure it out 21:56 pgimeno Isn't node metadata too slow to be used in rendering? 21:56 pgimeno no? 21:56 pgimeno wait, what? is that for real? if that's true, that would open a whole new world of possibilities 21:57 v-rob Well, there was a PR a while ago about storing a whole nodedef in metadata that worked at some point 21:57 celeron55_ until there's a strong need for it with lots of benefits that justify the performance downgrade that people want to start using, the nodes won't be extended just for the sake of extending them 21:57 celeron55_ v-rob: ...i think i made that, right? 21:58 v-rob Oh, did you? I didn't know 21:58 celeron55_ it created the option to make a node look completely different just by defining a custom node in metadata 21:58 celeron55_ or something like that 21:58 celeron55_ actually i don't remember, i'd have to look it up 21:59 celeron55_ and not by node id but literally a complete node definition 21:59 sfan5 https://github.com/celeron55/minetest/tree/meta_set_nodedef 21:59 celeron55_ there are probably updated versions of that by others 22:00 pgimeno I think that using metadata for rendering would be a revolution 22:00 DS-minetest #5919 22:00 ShadowBot https://github.com/minetest/minetest/issues/5919 -- [WIP] Meta-set nodedef: Part 4 by Thomas--S 22:02 celeron55_ looks like credit to me was lost at some point 8) 22:03 pgimeno imagine, being able to define textures independently per face, even mixing half-height nodes with different textures that the user can change for the top and the bottom 22:04 celeron55_ well, yeah, and then someone makes an entire world out of those and wonders why is it 100x slower to load than normal 22:05 celeron55_ of course the fact that someone will always abuse whatever is done isn't a reason not to do it 22:05 celeron55_ i don't remember why that branch remained as an experiment for me 22:05 v-rob I think the biggest problem aside from RAM/performance is that node definitions can easily become outdated, requiring cumbersome LBMs for compat 22:06 celeron55_ well of course 22:06 celeron55_ those should be kept for active nodes that will change their appearance from time to time anyway 22:06 pgimeno imagine a house made of 1 node walls and ceilings, with the floors different to the ceilings, with the inner walls different to the outer ones... these are the immediate examples that come to mind, and I'm not a creative person 22:07 celeron55_ altough, if you're happy with the appearance, how does it make sense to want it to update at any point in the future? 22:07 v-rob Well, #10801 + SSCSM + per-node textures would be awesome, but unlikely 22:07 ShadowBot https://github.com/minetest/minetest/issues/10801 -- Add CSM 2D Drawing API by v-rob 22:09 v-rob Inner/outer walls could easily be done with one content ID with #9193 22:09 ShadowBot https://github.com/minetest/minetest/issues/9193 -- Add paramtype2 for node texture variants 22:12 sfan5 while fully "dynamic" node appearance would obviously be very cool I think adding paramtype or metadata for specific parts of the nodedef is the most useful tradeoff 22:12 sfan5 just like that issue you linked 22:13 v-rob Nodes should be semi-dynamic-ish, not entities 22:14 pgimeno no, param2 doesn't give nearly the same flexibility with just 8 bits 22:15 v-rob That's kind of the reason why I started the whole discussion, since I think larger param2 would be useful 22:15 sfan5 ("or metadata") 23:51 Wuzzy #11110 23:51 ShadowBot https://github.com/minetest/minetest/issues/11110 -- Fix number of tool uses being off by 1..32767 by Wuzzy2 23:52 Wuzzy This PR has 2 days of work behind it.