Time  Nick           Message
01:14 ANAND          Approvals needed for simple bugfix PR #8342
01:14 ShadowBot      https://github.com/minetest/minetest/issues/8342 -- Correctly set visibility of local player's attached objects by PiezoU005F
01:14 ANAND          I've tested, and it works correctly.
05:59 Dead_Man       Okay, is this working now?
05:59 Dead_Man       I assume it is. Hello, all! Working on learning the rendering engine and pipeline so I can expand on it! First objective is shadows.
06:01 Dead_Man       I'll be sure to ask here if I have any questions.
06:33 ANAND          sfan5: Apparently, you'll have to prepend "fixes" to each individual issue number in https://github.com/minetest/minetest/pull/8832#issue-309269058, for all of them to be closed when the PR is merged.
06:39 ANAND          #8342 requires a rebase, but the author seems to be quite inactive on Github
06:39 ShadowBot      https://github.com/minetest/minetest/issues/8342 -- Correctly set visibility of local player's attached objects by PiezoU005F
06:40 ANAND          Their last activity seems to be this very PR, which was created on Mar 8
08:53 BuckarooBanzai ANAND: he quit mt a while ago :(
09:08 ANAND          Oh no
10:19 p_gimeno       wow, 8046 (now #8342) was not yet merged? I thought it already was. I came up with a very similar fix in https://github.com/stujones11/wield3d/issues/2#issuecomment-459990306
10:19 ShadowBot      https://github.com/minetest/minetest/issues/8342 -- Correctly set visibility of local player's attached objects by PiezoU005F
10:21 p_gimeno       I can see the frustration of not getting a very simple, straightforward, clearly right bugfix sitting there for so many months
10:22 p_gimeno       -not
10:29 sfan5          ANAND: please test #8342 again
10:29 ShadowBot      https://github.com/minetest/minetest/issues/8342 -- Correctly set visibility of local player's attached objects by PiezoU005F
13:38 pmp-p          i think there's something wrong here https://github.com/minetest/minetest/blob/master/src/client/renderingengine.cpp#L325  regarding issue #8841
13:38 ShadowBot      https://github.com/minetest/minetest/issues/8841 -- rogue behaviour on mate-desktop (marco WM +compositing)
13:38 pmp-p          metacity is complaining and it should not
13:39 pmp-p          so there may be a problem MT side
13:48 pmp-p          when building again GLES + x11 there's a small final linking problem since link.txt will miss a -lGL with the -lEGL
14:51 ANAND          sfan5: See https://github.com/minetest/minetest/pull/8342#issuecomment-524345134
14:51 ANAND          Something seems to be wrong
14:53 ANAND          Some entities are showing up properly while the others aren't
14:59 sfan5          the only difference I can see between the old and new code is that updateAttachments() is not called after m_is_visible was called
14:59 sfan5          s/called$/changed/
16:26 * rubenwardy   considers adding meta data support to schematics
16:26 rubenwardy     also, built-in support for schematic mapgen - pretty please?
16:32 rubenwardy     also, support for baked in lighting for schematics would be nice
16:32 rubenwardy     urgh
16:32 p_gimeno       sfan5: it looks to me like m_attached_to_local is not up to date at that point, because it was moved to updateAttachments()
16:34 p_gimeno       it was moved there in 4aa9a669cb184b77213e8df82eb20eda5aad9004
16:38 p_gimeno       oh forget that, setAttachment calls updateAttachments
16:49 p_gimeno       funny, the patch works for me with wield3d but not with 3d_armor... ANAND could you confirm that?
16:51 Krock          optimally all attachment-relevant variables should be changed in setAttachment() or updateAttachment()
16:51 Krock          and if that's not possible, add a flag so it does not overwrite that the next time the attachments update
16:53 Krock          rubenwardy: sounds good. save the entire mapnode, including metadata if requested
16:53 Krock          thing is that metadata tends to make schematics huge due to lack of compression
16:54 p_gimeno       actually, the patch works except the first time. Switching to another tool and back makes it show and then it keeps working.
16:54 p_gimeno       (in 3d_armor)
16:59 p_gimeno       ANAND: I don't know how to see the health gauge, is that a new version of 3d_armor or something?
16:59 Krock          separate mod. gauges.
16:59 p_gimeno       k
17:00 rubenwardy     I suggest storing the metadata in a footer
17:00 rubenwardy     And compressing either just the footer or the whole thing
17:00 rubenwardy     Maybe we should also have xmts for compressed schematics
17:01 Krock          so mts aren't forwars compatible?
17:01 Krock          #8846 works. will merge after #8812 in 15 minutes
17:01 ShadowBot      https://github.com/minetest/minetest/issues/8846 -- Fix default hand definition not using wieldhand.png by Wuzzy2
17:01 ShadowBot      https://github.com/minetest/minetest/issues/8812 -- [NO SQUASH] Occlusion: Code cleanup and better checks using "light_propagates" by SmallJoker
17:07 p_gimeno       ANAND: I can't reproduce the behaviour you report in https://github.com/minetest/minetest/pull/8342#issuecomment-524345134
17:10 p_gimeno       the gauge looks displaced though as it goes through my body, but that's not related to that PR, it happens in master to
17:15 Krock          I'm looking for the term to describe: auto inline_function = [=] (type *param) { .. }
17:16 Krock          merging the prs..
17:17 Krock          p_gimeno: because it wasn't made for 5.0 models
17:17 p_gimeno       ah
17:18 sfan5          rubenwardy: what are you referring to?
17:28 sfan5          >		check.Y = CLOSEST_EDGE(pos_camera, block_bounds, Y);
17:28 sfan5          Krock: is this alright with a macro for you?
17:29 Krock          yes, optionally also with "check" since it's got to be the same axis
17:29 Krock          but that's fine too
17:30 Krock          well, not necessary to check against the mapblock center in what currently exists as occlusion culling shootlines
17:30 Krock          but would be *technically* correct
17:36 sfan5          whoops i just spotted two errors in my pr
18:07 Krock          for debugging it should be possible to draw lines like it's done for the pointed selectionbox border
18:07 Krock          to see where the occlusion actually goes
18:35 Krock          I'd like to do some performance testing in #8838 before it's merged
18:35 ShadowBot      https://github.com/minetest/minetest/issues/8838 -- Make Mapgen::spreadLight use a queue by DS-Minetest
18:36 Krock          mainly comparing it with #8832
18:36 ShadowBot      https://github.com/minetest/minetest/issues/8832 -- Mapgen: Run light spread algorithm iteratively by sfan5
18:36 sfan5          sure
18:37 sfan5          neither seem to change the performance much, so you need a specific reproducable scenario to test the exact values
18:38 Krock          well, dummy backend should be enough
18:42 lhofhansl      hi all... Planning to merge #8838 soon.
18:42 ShadowBot      https://github.com/minetest/minetest/issues/8838 -- Make Mapgen::spreadLight use a queue by DS-Minetest
18:43 sfan5          another thing is that the other PR queues an entire area worth of light spreading before doing actual spreading
18:43 sfan5          lhofhansl: Krock wanted to do some performance testing first
18:43 lhofhansl      Ok... I'll hold off.
18:43 lhofhansl      BTW. testing #8812 / #8844 now.
18:43 ShadowBot      https://github.com/minetest/minetest/issues/8812 -- [NO SQUASH] Occlusion: Code cleanup and better checks using "light_propagates" by SmallJoker
18:43 ShadowBot      https://github.com/minetest/minetest/issues/8844 -- Improve occlusion culling by sfan5
18:44 sfan5          nice, thanks
18:45 Krock          lhofhansl: fine, go ahead. sfan5's method lost by far
18:45 Krock          https://pastebin.com/raw/qSFa2PML
18:46 sfan5          oh it's that much slower
18:46 lhofhansl      On that. In the past I had experimented improving the accuracy by sending more rays (8 more, 1/2 between the block's center and the corner). Perhaps something to consider again.
18:46 sfan5          it it the memory allocations? ?
18:46 Krock          yeah. I really wonder what's causing that
18:46 Krock          well, there must be less insertions in DS' version
18:46 Krock          the queue is shorter, or no duplicates
18:46 lhofhansl      Cool. Do you normally merge via Github, or manually apply the patch? (I seen both recommended here)
18:47 sfan5          usually github these days
18:47 lhofhansl      Aye. Will do. I think DS' version is faster because it avoids recalculations.
18:48 lhofhansl      (with high light levels)
18:48 Krock          that or queue.pop_front(); is expensive
18:48 sfan5          std::queue just wraps std::deque and calls pop_front() so that would be identical
18:49 Krock          indeed, just seen that too
18:49 Krock          pre-allocating the queue memory would surely help here
18:50 sfan5          it would also be quite funny to have a container called "queue" where pop/push aren't O(1)
18:50 Krock          since it's probably deallocating in pop() and allocating in emplace() a few times
18:51 Krock          unless there's some threshold internally to indicate the upper limit for bulk-allocate and lower limit for bulk-deallocate
18:53 Krock          so why not a dequeue directly?
18:53 Krock          testing that
18:54 lhofhansl      Alright. Won't merge for now.
19:00 lhofhansl      So #8812... The straight line special case is pretty awesome. I do find, though, that there are some new scenarios when you look just sideways along a straight tunnel that more blocks are occluded when they should not.
19:00 ShadowBot      https://github.com/minetest/minetest/issues/8812 -- [NO SQUASH] Occlusion: Code cleanup and better checks using "light_propagates" by SmallJoker
19:00 lhofhansl      Perhaps we should have a version first that leaves everything functionally unchanged, but adds the straight line special case...?
19:01 sfan5          are you referring to 8844, since 8812 is already merged?
19:01 lhofhansl      I tested them together :)
19:01 lhofhansl      Perhaps too late, and we fix the issues as we go.
19:02 lhofhansl      Lemme pull and test with 8812 alone.
19:03 sfan5          the occlusion behaviour will most certainly be worse
19:04 lhofhansl      As in it will incorrectly cull more blocks?
19:04 sfan5          yes
19:05 lhofhansl      Hmmm... Why's that a good thing?
19:07 lhofhansl      (It is indeed much worse in the scenario I tested)
19:18 lhofhansl      Apologies for only really having time to look now... But this is way-way worse (at least without 8844). Could we not have done the refactoring without functional changes?
19:19 Krock          testing with ringbuffer. deque turned out to be even worse
19:20 Krock          or comparable, if done right
19:20 sfan5          lhofhansl: yes it could have been done, but we kinda wanted to anyway
19:21 sfan5          also I am well aware of the "way-way-worse" bit, I planned 8844 to be merged to bring occlusion culling much closer to how it performed before
19:21 lhofhansl      I propose we undo that change. Commented to that extend on the PR.
19:23 Krock          even a C-style static ringbuffer takes double the time?!? there must be some GCC optimizations that favour argument-passed data
19:24 sfan5          maybe "occlusion check stops 27 nodes before target block" should be made a feature
19:25 lhofhansl      If it improves the visual... :)  The old code was more precise at close ranges and gets more coarse at a distance. I get the desire to cull more, but not if it does not look right.
19:27 sfan5          "more precise", well it just didn't cull anything that was close
19:29 sfan5          I do think even if the occlusion behavior is returned to how it was before that 8844 would make an useful addition
19:29 Krock          https://pastebin.com/raw/qSFa2PML updated Ctrl + F5
19:30 Krock          weird stuff. there's now no longer memory allocations in the ringbuffer
19:31 lhofhansl      The server is also culling (i.e. not sending culled blocks). The client number is not necessarily the full story.
19:31 lhofhansl      I'm all for 8844, it's an awesome addition!
19:32 lhofhansl      DS' version is magic :)(
19:32 Krock          I seriously don't get why it's faster
19:33 lhofhansl      (Now I do not get it anymore either. Is it still correct?)
19:34 lhofhansl      sfan5: Let's add another PR to bring the functionality back to what it was. Then merge 8844. Then look for further ways to improve this (8812 makes that easier).
19:41 sfan5          alright
19:43 lhofhansl      Unfortunately with the tracing we do there's always the trade-off between performance and accuracy... :(
19:43 lhofhansl      Minecraft does this: https://tomcc.github.io/2014/08/31/visibility-1.html
19:43 lhofhansl      (Not that we should do that, too, but it's interesting)
19:44 Krock          Q: what spreads the light client-side?
19:45 Krock          not just that. if torches are placed after mapgen, it's not spreadLight that is called
19:45 Krock          * lightSpread
19:46 lhofhansl      It's not? It has to be done server side, no? Or other clients won't see it...
19:47 Krock          voxelalgorithms spread_light
19:49 Krock          that's basically triple the code of mapgen, except that mapgen has its own implementation
19:49 sfan5          oof
19:49 Krock          guys you're optimizing at the wrong place I guess
19:50 sfan5          well the main idea was to fix segfaults due to stack overflow, the optimization idea came later
19:51 lhofhansl      Still nicer code and faster. I'd still say let's merge.
19:51 sfan5          of course
19:52 lhofhansl      DS' improved it even further :)
19:52 Krock          add a comment referencing to voxelalgorithms
19:52 Krock          it shall not be forgotten
19:52 lhofhansl      +1
19:53 lhofhansl      Gotta head out in a bit. If it's hard to fix, let's revert 8812 and regroup. I very much like the refactoring of 8812 and 8844 is definitely a vast improvement for tunnels!
19:54 sfan5          lhofhansl, Krock: https://github.com/minetest/minetest/pull/8850
19:55 lhofhansl      Looking...
19:56 lhofhansl      Testing...
20:00 lhofhansl      Cool. I guess we can leave the step = 1.2 and stepFac = 1.05 (it was 1.0 and 1.1 resp, before). I can't see much a difference to before.
20:00 Krock          so if (distance < BLOCK_DIAGONAL) isn't enough?
20:00 Krock          stepfac has to be small. 1.1 is way too high
20:01 lhofhansl      Depends on what the goal is. Close blocks fill more of the view-area so being precise there is important, as we go into the distance the culling becomes less and less effective anyway,
20:02 Krock          oh I see. step might be higher than BLOCK_DIAGONAL so that it's never reached
20:02 Krock          s/step/offset/
20:02 Krock          s/.*//
20:02 lhofhansl      I.e. a smaller starting step and then a larger stepFac. But... as I said I didn't see a difference in behavior is my tests.
20:03 Krock          a smaller step (towards 1.0) results in more node lookups, hence either better occlusion checked blocks or more false-positibes
20:03 Krock          *positives
20:04 lhofhansl      (btw. I had a PR decreasing stepFac before, for some reason we decide to revert it, but I forgot why. I agree smaller is more accurate, obviously)
20:04 lhofhansl      8850 looks good to me.
20:07 Krock          lhofhansl: stepfac = 1.0f  and master/HEAD should actually result in the same thing
20:07 lhofhansl      You mean step = 1.0f. stepfac should always be > 1.0.
20:08 lhofhansl      (Unless we do not want to decrease accuracy with distance)
20:08 Krock          step = 2, stepfac = 1
20:08 Krock          former * BS ofc
20:10 lhofhansl      As an alternative we could *only* check close blocks and just stop after a certain radius. Oh well... That's for other PRs.
20:14 sfan5          is #8838 ready for merge then?
20:14 ShadowBot      https://github.com/minetest/minetest/issues/8838 -- Make Mapgen::spreadLight use a queue by DS-Minetest
20:15 lhofhansl      +1
20:15 Krock          yes
20:16 sfan5          merging it then
20:18 sfan5          done that
20:18 sfan5          8844 is also updated (depends on 8850 being merged first)
20:20 sfan5          Krock: any outstanding issues with either of those? ^
20:22 Krock          former surely needs testing, regarding the latter I'm still don't know what's going wrong
20:22 lhofhansl      In 8838 we do not have the comment pointing to voxelalgorithms...
20:23 Krock          mhm well so
20:24 sfan5          Krock: i think what is going wrong there are the (up to) 11 nodes that are closer than BLOCK_DIAGONAL but not within block bounds
20:24 sfan5          those make the difference in culling here
20:25 sfan5          well it's up to 27 actually depending on which corner you check, up to 19 for the center
20:25 Krock          that ^
20:26 Krock          so in fact it can skip an entire mapblock
20:31 Krock          oh wait I got it
20:32 lhofhansl      No objections to 8850 and 8844 from me.
20:33 Krock          in a corridor it's easier to hit the wall in between the pos_target and pos_camera. ignoring up to 27 nodes allows far away mapblocks to be drawn where otherwise would be solid nodes in the last few meters
20:34 Krock          the IMO sane solution for this would be to alter "count" depending on distance
20:35 Krock          because nearby nodes have a higher occlusion culling factor than far away ones
20:35 lhofhansl      That's a good idea. The fact count itself is just heuristic and get it mostly right. I tried to remove it before and improve accuracy instead and it turned out it was too hard to tune the rest accordingly.
20:39 lhofhansl      Or have some other way to ensure that each node is guaranteed to be consulted at most once.
20:40 lhofhansl      Alright... Gotta head. I'm all fine with merging 8850 and 8844 (added my One Approval label).
20:41 Krock          uh well, I'd prefer to see "count" to be changed rather than adding this no-check-hack
20:46 sfan5          the question is how hard is it to get that adjustment exactly right
20:48 Krock          worst case is known. 1x2 corridor, between mapblock center and mapblock edge
20:48 sfan5          I don't disagree, but it would be better to make it works equally good before (or even better with 8844) and then when someone takes the time to fix it properly, merge it then
20:49 sfan5          good as before*
20:50 Krock          +1 if you want to restore the last-good state. +2 if you'd have the time to fix "count"
20:50 Krock          or me.. would need to test that tomorrow if so
20:51 sfan5          I think 8844 improves occlusion culling in a more user visible way than fixing "count" to be somehow more accurate, so I didn't plan on doing the latter
21:04 Krock          count += MYMIN(1.0f, 20.0f / offset);  works for me
22:46 lhofhansl      Back... Let's restore the current behavior (more or less) with 8850. Then we can improve stuff (such as 8844, and Krock
22:46 lhofhansl      's count fix)
23:25 sfan5          agree, will be merging #8850 in 15 minutes
23:25 ShadowBot      https://github.com/minetest/minetest/issues/8850 -- Restore approximate occlusion check by sfan5