Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: glsl/normal: use ifdef for normal z reconstruction #189

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@illwieckz
Copy link
Member

commented Mar 24, 2019

DO NOT MERGE until the #ifdef bug is fixed, see #190.

This is a simple change that makes glsl relying on #ifdef instead of an uniform to test for the fact the normalmap is embedding heightmap in alpha channel or not. This makes GMA965 able to render parpax default scene with 11 fps instead of 10 (+10%) (unnoticeable on R9 390X).

This is not merged yet because of a nasty bug, it looks like the define is not defined any time as while I move in map, the wrong code is sometime run instead of the expected one:

glsl ifdef

glsl ifdef

The bug is on the wall texture, not on the blooming reactor.

I reproduced the bug on both radeonsi (R9 390X) and i965 (Haswell) drivers, this is likely to not be a driver bug but a bug in dæmon code, the ifdef code is very cryptic and would benefit a lot from a rewrite.

I'm basically pushing this branch so people have code to debug the #ifdef bug (#190).

@slipher

This comment has been minimized.

Copy link
Contributor

commented Mar 25, 2019

A couple suspicious things:

  • You only set the height mapping macro if the parallax cvar is enabled, but some of the GLSL that depends on it is outside of ifdef parallax blocks.
  • Liquid shader has the macro but it is never enabled.
@illwieckz

This comment has been minimized.

Copy link
Member Author

commented Mar 25, 2019

You only set the height mapping macro if the parallax cvar is enabled, but some of the GLSL that depends on it is outside of ifdef parallax blocks.

good catch!

Liquid shader has the macro but it is never enabled.

right, this code is not yet used but it's better to not have bugs in dormant code

I'll reread the stuff to be sure this variable is set in any code that use normal map (they may be more), not only those that use parallax.

@illwieckz

This comment has been minimized.

Copy link
Member Author

commented Mar 25, 2019

I postpone changes to this PR for after #188 merge since once #188 is merged I would be able to do things like that:

if (normalMapping || parallaxMapping)

@illwieckz illwieckz force-pushed the illwieckz:ifdef-heightmap branch from be340e6 to d87652a Mar 30, 2019

@illwieckz

This comment has been minimized.

Copy link
Member Author

commented Mar 30, 2019

I rebased on master but the #190 bug is still there so merge is blocked

@slipher

This comment has been minimized.

Copy link
Contributor

commented Mar 31, 2019

It looks like in some shaders, you are doing SetHeightMapInNormalMap only if a condition is met. So when the condition is not met, you get a random value left over from the last time the shader was used.

@illwieckz illwieckz force-pushed the illwieckz:ifdef-heightmap branch 2 times, most recently from 523a6fb to 48a0893 Apr 20, 2019

@illwieckz

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2019

do someone can explain me why the latest commit is fixing the bug?

@illwieckz illwieckz force-pushed the illwieckz:ifdef-heightmap branch from 48a0893 to e566f7b Apr 20, 2019

@slipher

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2019

Do you really need the 2nd commit for it to work? It looks like the first one was changed to avoid the bug that I talked about before.

@illwieckz

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2019

Yes, the second commit is required to fix the bug. The changes I added to the first commit to fix the issue you talked about before is not enough.

It's also why I kept this second commit separate to be able to ask the question. 😛

@illwieckz illwieckz force-pushed the illwieckz:ifdef-heightmap branch from e566f7b to 9999400 Apr 20, 2019

@illwieckz

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2019

Well, that only fixes it for now, if I add some commits editing some unrelated stuff the bug comes back again… the unrelated commit I was working on was a commit merging reflection in lightmap stage to avoid double parallax compute… this one added a macro since the reflectCube glsl would be useless in that case, and the bug came back… I know the macro thing is tricky since each macro is a bit of one integer, may be we overflow that integer because of “too many macro”?

There is a check about that “too many macro” scenario but I never triggered it so I suspect it does not work. In some wip earlier commits I had a lot of macros and I was wondering why the "too many macro" test was not shooting at me…

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.