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

R_UploadImage: detect heightmap in normalmap alphachanel #171

Merged
merged 1 commit into from Mar 9, 2019

Conversation

Projects
None yet
2 participants
@illwieckz
Copy link
Member

illwieckz commented Feb 27, 2019

detect heightmap in normalmap alpha channel and enable parallax for the given shader

this makes removes the need of an explicit shader keyword telling the engine there is an heightmap embedded in normalmap: if there is an alpha channel in normalmap there is an eightmap, else there is only a normalmap

was experimented first in #159

@slipher

This comment has been minimized.

Copy link
Contributor

slipher commented Mar 4, 2019

Instead of storing the information in the horrible bits field, how about adding a new one like bool parallaxDetected.

@illwieckz

This comment has been minimized.

Copy link
Member Author

illwieckz commented Mar 4, 2019

I'm not sure it would be better, especially since the same texture file contains two data it's not a bad idea by itself.

@slipher

This comment has been minimized.

Copy link
Contributor

slipher commented Mar 4, 2019

The imagebits stuff is very semantically confused. It seems to mostly be used as a way of telling the image loading code how to behave, but then sometimes the image loading code may add in its own flags. It's all too messy, so it's bad to add additional uses.

The IF_DISPLACEMAP flag is currently used in the case of an explicit heightmap shader keyword, to instruct the image loader to keep the alpha channel (in that one weird case we discussed above). So it should not be abused for the separate case of reporting whether a heightmap was automatically detected.

@illwieckz

This comment has been minimized.

Copy link
Member Author

illwieckz commented Mar 4, 2019

This flag is not only used in that weird case. The primary usage of this flag is to tell the renderer there is an heightmap in normalmap alpha channel whatever the way it is known. It was formerly known exclusively via a shader keyword, now it can be known via content detection. In fact there is nothing to do with the information of an heightmap being automatically detected or not, the only useful information is the presence of an heightmap or not.

So if we want to add an extra boolean we can add it only to tell that weird use case we discussed above to uses 4 samples instead of 3 and to set this flag in any way, since the renderer needs this flag to render the heightmap itself. Also, in case of a shader containing the explicit shader keyword this boolean would have been set without requiring the need for the flag to be set, manually or automatically. It would just add some weirdness to the code.

I don't think it's a good idea to tweak that behavior if it's not required by a feature, for example to provide support for loose heightmaps. That's something that would be cool to have and this would perhaps require a revamp, but until there it's ok.

@illwieckz

This comment has been minimized.

Copy link
Member Author

illwieckz commented Mar 4, 2019

note to myself: it would be possible to test for loose heightmap this way:

if ( image->bits & IF_DISPLACEMAP || !( image->bits & IF_NORMALMAP ) )

@illwieckz illwieckz force-pushed the illwieckz:detectheightmap branch from d90cff3 to 7bef0d9 Mar 4, 2019

@illwieckz

This comment has been minimized.

Copy link
Member Author

illwieckz commented Mar 4, 2019

@slipher, hmm sorry I'm wrong, that bit seems to not be used elsewhere…
Well, I don't know what's better to do.

@slipher

This comment has been minimized.

Copy link
Contributor

slipher commented Mar 4, 2019

Loose heightmaps aren't actually supported, are they? We don't have to worry about breaking a feature that doesn't exist.

@illwieckz

This comment has been minimized.

Copy link
Member Author

illwieckz commented Mar 4, 2019

Loose heightmaps are not supported, but if a solution keeps the door open while not costing more, it's not bad. In any way you seem right about the boolean. The renderer relies on a boolean, not that bit stuff, so we may use a boolean from the start.

@illwieckz

This comment has been minimized.

Copy link
Member Author

illwieckz commented Mar 4, 2019

Well, I think the current code is simple enough, especially since doing things another way would require to pass a boolean to R_UploadImage. The good thing with the current implementation is that function definition remains the same.

I'm not against a revamp of such things by itself, but I think it would fit better in another PR.

@illwieckz

This comment has been minimized.

Copy link
Member Author

illwieckz commented Mar 4, 2019

So the code looks ok to me now. If the only concern is that we may use booleans instead of that bits stuff, it's ready to merge.

R_UploadImage: detect heightmap in normalmap alphachanel
detect heightmap in normalmap alpha channel
and enable parallax for the given shader

@illwieckz illwieckz force-pushed the illwieckz:detectheightmap branch from 7bef0d9 to fba1c87 Mar 9, 2019

@illwieckz illwieckz merged commit fba1c87 into DaemonEngine:master Mar 9, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.