Time |
Nick |
Message |
00:01 |
|
harrison joined #minetest-dev |
00:10 |
|
Cornelia joined #minetest-dev |
00:36 |
|
pauloue joined #minetest-dev |
00:38 |
|
pauloue joined #minetest-dev |
00:51 |
|
Cornelia joined #minetest-dev |
01:02 |
|
Cornelia joined #minetest-dev |
01:07 |
|
pauloue joined #minetest-dev |
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. |
01:31 |
|
Megaf joined #minetest-dev |
01:40 |
|
Cornelia joined #minetest-dev |
01:58 |
|
Cornelia joined #minetest-dev |
03:58 |
|
behalebabo joined #minetest-dev |
04:22 |
|
DeadMan2 joined #minetest-dev |
04:27 |
|
atlas_ joined #minetest-dev |
04:36 |
|
atlas_ joined #minetest-dev |
04:37 |
|
atlas_ left #minetest-dev |
05:53 |
|
Dead_Man joined #minetest-dev |
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:00 |
|
Lone-Star joined #minetest-dev |
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 |
07:05 |
|
fluxflux joined #minetest-dev |
07:26 |
|
calcul0n joined #minetest-dev |
08:23 |
|
Player-2 joined #minetest-dev |
08:31 |
|
Lone-Star joined #minetest-dev |
08:53 |
BuckarooBanzai |
ANAND: he quit mt a while ago :( |
08:54 |
|
ssieb joined #minetest-dev |
09:08 |
ANAND |
Oh no |
09:15 |
|
calcul0n joined #minetest-dev |
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 |
|
ssieb joined #minetest-dev |
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:27 |
|
Fixer joined #minetest-dev |
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 |
11:08 |
|
proller joined #minetest-dev |
12:31 |
|
Cornelia joined #minetest-dev |
12:52 |
|
Cornelia joined #minetest-dev |
12:59 |
|
Megaf joined #minetest-dev |
13:02 |
|
Wuzzy joined #minetest-dev |
13:06 |
|
behalebabo joined #minetest-dev |
13:37 |
|
Lia joined #minetest-dev |
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:07 |
|
behalebabo joined #minetest-dev |
14:13 |
|
TC01 joined #minetest-dev |
14:42 |
|
Lone_Wolf joined #minetest-dev |
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/ |
15:07 |
|
Amaz joined #minetest-dev |
15:31 |
|
TC01 joined #minetest-dev |
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:43 |
|
Krock joined #minetest-dev |
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:11 |
|
ssieb joined #minetest-dev |
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:08 |
|
lllI1I joined #minetest-dev |
18:14 |
|
Fixer_ joined #minetest-dev |
18:16 |
|
tomraceror joined #minetest-dev |
18:29 |
|
behalebabo joined #minetest-dev |
18:33 |
|
HDMI_STECKDOSE joined #minetest-dev |
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 |
|
HDMI_STECKDOSE joined #minetest-dev |
18:37 |
sfan5 |
neither seem to change the performance much, so you need a specific reproducable scenario to test the exact values |
18:38 |
|
tomraceror joined #minetest-dev |
18:38 |
Krock |
well, dummy backend should be enough |
18:42 |
|
lhofhansl joined #minetest-dev |
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:51 |
|
GreenDimond joined #minetest-dev |
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 |
|
HDMI_STECKDOSE joined #minetest-dev |
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:11 |
|
paramat joined #minetest-dev |
19:17 |
|
behalebabo joined #minetest-dev |
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 |
|
proller joined #minetest-dev |
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:51 |
|
GreenDimond left #minetest-dev |
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:12 |
|
lllI1I joined #minetest-dev |
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:40 |
|
lhofhansl left #minetest-dev |
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 |
21:43 |
|
paramat joined #minetest-dev |
22:09 |
|
behalebabo joined #minetest-dev |
22:43 |
|
lhofhansl joined #minetest-dev |
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) |
22:57 |
|
lhofhansl left #minetest-dev |
23:19 |
|
Cornelia joined #minetest-dev |
23:24 |
|
rom1504 joined #minetest-dev |
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 |
23:27 |
|
paramat joined #minetest-dev |