Skip to content

normal map attenuation properties #1461

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

Open
wants to merge 1 commit into
base: protocol_changes
Choose a base branch
from

Conversation

HifiExperiments
Copy link
Member

closes #1139

per the open question in the issue, I didn't yet implement a mode for this. do we want to be able to set this to "inherit" etc.? if that's something we want I can add it, these properties are just slightly different than other zone properties so not sure?

Funding

This project is funded through NGI Zero Core, a fund established by NLnet with financial support from the European Commission's Next Generation Internet program. Learn more at the NLnet project page.

NLnet foundation logo
NGI Zero Logo

@HifiExperiments HifiExperiments added question Further information is requested needs CR This pull request needs to be code reviewed needs QA This pull request needs to be tested protocol change NLnet labels Apr 17, 2025
@JulianGro
Copy link
Member

This might be a weird question to ask after you open this PR, but why should we expose the normal map attenuation?
Is it a tradeoff thing, like with the shadow bias, where there is no "good" setting?

@HifiExperiments
Copy link
Member Author

fair question! yes it’s kinda like the shadow settings. normal maps are for small details which become high frequency noise at a distance, causing artifacts like moire patterns. we fade them out over a distance to hide this, but the distance at which we want to fade can depend on the scale of the objects/normal maps, so it’s nice to make it configurable for creators

other engines have similar things I believe

@JulianGro
Copy link
Member

Sounds like the distance should be set based on render resolution and how large a normal map is rendered in relation to its resolution (or how detailed it is, if we can find that out).

@ksuprynowicz
Copy link
Member

Currently lack of normal maps on distant objects makes normal mapped terrain look very flat, and having this setting is a very welcome improvement that was requested by several artists. I think it's a really good idea to allow it to "Inherit" like other settings.
@HifiExperiments Maybe the normal map noisiness in the distance is caused by mipmap bias?

Copy link
Member

@ksuprynowicz ksuprynowicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Everything looks good :)

@ksuprynowicz ksuprynowicz added CR approved This pull request has been successfully code reviewed and removed needs CR This pull request needs to be code reviewed labels Apr 18, 2025
@ksuprynowicz
Copy link
Member

I just tested it and everything works very well, but currently there's no Inherit functionality, so it makes using this feature very difficult. I think zones should inherit it by default, like skyboxes for example.
Another thing is that I didn't notice any adverse effects of setting this to a very high value, world looks much less flat that way. Maybe we should set it to a very high value by default, so that users are not confused why normal maps disappear in the distance?
@AleziaKurdis Your opinion on this would be really appreciated, especially since your worlds use normal maps extensively.

@ksuprynowicz
Copy link
Member

In my opinion it looks way better with limit set to greater distance, for example in Alezia's Kopra world:
Default distance:
overte-snap-by--on-2025-04-18_22-38-12

Increased distance:
overte-snap-by--on-2025-04-18_22-39-28
overte-snap-by--on-2025-04-18_22-39-41
overte-snap-by--on-2025-04-18_22-39-56

@AleziaKurdis
Copy link
Contributor

I just tested it and everything works very well, but currently there's no Inherit functionality, so it makes using this feature very difficult. I think zones should inherit it by default, like skyboxes for example. Another thing is that I didn't notice any adverse effects of setting this to a very high value, world looks much less flat that way. Maybe we should set it to a very high value by default, so that users are not confused why normal maps disappear in the distance? @AleziaKurdis Your opinion on this would be really appreciated, especially since your worlds use normal maps extensively.

Inheritance would be good to have.
But not sure we need to have s specific node for it.
Maybe if the properties was in the Keylight node that would be enough. As for shadows stuff. (just an idea, let me know what you are thinking about it.)
Not sure I would risque to change the default value. It will affect existing content.
I supposed that if there was a limit that it has a rendering cost when we increase it? It would be nice to know how much.
Aslo, is there a natural distance limit where the normal map just stop to be renderable? or it can be allied what ever the distance?

@ksuprynowicz
Copy link
Member

I supposed that if there was a limit that it has a rendering cost when we increase it?

There doesn't seem to be any performance decrease, I think shader always samples normal map?

Aslo, is there a natural distance limit where the normal map just stop to be renderable?

I don't think so. The worlds I tested looked much better with longer normal map distance, and normal maps become softer in the distance naturally thanks to mipmaps.

Not sure I would risque to change the default value. It will affect existing content.

True. Considering amount of existing content though, maybe it would be best to test and see if there are any problems, and if exisitng worlds look better or worse with longer distance? I'd be happy to help testing and gather comparison screenshots.

@AleziaKurdis
Copy link
Contributor

If there is no real rendering cost impact. Then I see no reason to have this as a property. There is not artistic value to reduce that "normal map "rendering distance.
So I said just let the mipmaps rules the game.

So inheritence would become pointless too.

Maybe turn this as an interface graphical setting (if it's not already that)
and have it high by default.
At second thought, the existing limit was more a visual punishment than other things

@ksuprynowicz
Copy link
Member

I will test a bit more and see if I can find world where attenuation is needed - for now it looks like having normal maps at all distances looks much nicer in most worlds.

@HifiExperiments
Copy link
Member Author

HifiExperiments commented Apr 18, 2025

I think the problematic case is going to be more like:

  • eye level with terrain
  • normal maps are high frequency/small
  • (edit) while you’re moving

in those cases the mip maps start to break down. trilinear blending would probably help but not completely. if I remember correctly hifi ran into this problem for some event or environment, hence the introduction of the fade out

so if that were true, even if it’s not used much, seems like we’d need a property for it. but can certainly bump up the defaults (there’s no cost, like 74 said), and definitely add inheritance (would add a new mode)

but if that’s NOT true and we decide it’s not necessary, I can certainly make it just a graphics setting or something (and still bump up defaults)

@ksuprynowicz
Copy link
Member

@HifiExperiments That's a really good point, I need to test more and see what happens at sharply angled surfaces

@ksuprynowicz
Copy link
Member

Is it possible that we are already using anisotropic filtering for mipmap textures?
image

@HifiExperiments
Copy link
Member Author

oh! possibly:

theTexture = gpu::Texture::create2D(formatGPU, image.getWidth(), image.getHeight(), gpu::Texture::MAX_NUM_MIPS, gpu::Sampler(gpu::Sampler::FILTER_MIN_MAG_MIP_LINEAR));

{ GL_LINEAR_MIPMAP_LINEAR, GL_LINEAR }, //FILTER_MIN_MAG_MIP_LINEAR,

uint32 _maxAnisotropy = 16;

but…is that sufficient?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CR approved This pull request has been successfully code reviewed needs QA This pull request needs to be tested NLnet protocol change question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants