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

Fix Rendering Issue in PBRTerrain #2365

Merged
merged 6 commits into from
Feb 10, 2025
Merged

Fix Rendering Issue in PBRTerrain #2365

merged 6 commits into from
Feb 10, 2025

Conversation

yaRnMcDonuts
Copy link
Member

@yaRnMcDonuts yaRnMcDonuts commented Feb 6, 2025

This PR attempts to fix a rendering bug that a few jme users have experienced on certain devices, as discussed in this thread: https://hub.jmonkeyengine.org/t/requesting-help-troubleshooting-pbrterrain-bug/47895

I believe the issue was due to a combination of things that were done incorrectly in the process of reading normal maps, calculating tangents, and then blending the normal values.

I believe it is also important to normalize the normal value prior to blending each layer, which I previously was not doing.

There's also some code that is necissary for putting a normal map in the proper range, and this was being done improperly for tri-planar normal mapping. It should be done 3 times (once per read) and then normalized, and this was incorrect previously.

Once this PR is done I'll post back to that thread and request testing from the users who were experiencing the bug on their device.

This PR attempts to fix a rendering bug that a few jme have experienced on certain devices, as discussed in this thread:
https://hub.jmonkeyengine.org/t/requesting-help-troubleshooting-pbrterrain-bug/47895

I believe the issue was due to a combination of things that were wrong with the process of reading normal maps, calculating tangents, and then blending them.

The order of these operations is important, and I believe it is also important to normalize the normal value prior to blending each layer, which I previously was not doing.

There's also some code that puts a normal map in the proper range, and this was being done improperly for tri-planar normal mapping.

Once this PR is done I'll post back to that thread and request testing from the users who were experiencing the bug on their device.
@yaRnMcDonuts
Copy link
Member Author

I plan to merge this in the next 24 - 48 hours

@yaRnMcDonuts yaRnMcDonuts merged commit 98c0f9d into master Feb 10, 2025
15 checks passed
@stephengold stephengold added this to the v3.8.0 milestone Feb 23, 2025
@stephengold
Copy link
Member

@yaRnMcDonuts: perhaps it's time to delete the branch

@yaRnMcDonuts yaRnMcDonuts deleted the yaRnMcDonuts-patch-7 branch February 24, 2025 02:38
@yaRnMcDonuts
Copy link
Member Author

I deleted the branch, and also went back through and deleted the branches in a couple other PRs where I forgot to do so.

I also realized I've been forgetting to flag some PRs to the 3.8.0 milestone recently, but I'll make sure I am remembering to do this going forward. Thanks for flagging the PRs I've missed in the meantime!

@stephengold
Copy link
Member

@yaRnMcDonuts: You're welcome.

I think it's particularly important to make the milestones accurate before creating a release branch, since that's when the meaning of "future release" suddenly changes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants