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

improves groundcover view distance #3219

Merged
merged 35 commits into from
Nov 8, 2021
Merged

Conversation

bosvensson1
Copy link
Contributor

This PR aims to solve all issues with Groundcover view distance handling in a satisfying way while preserving other optimisations that benefit other features. The main idea here is not to rely on ViewData updates for distance culling calculations because we can not accurately determine distance against lazily updated views. Instead, we perform an accurate measurement in a cull callback we can run every frame in Groundcover itself. While we do have to add some code to handle this feature, it is quite loosely coupled code that could be useful elsewhere in the future. These changes should address a performance regression @akortunov experienced.

@bosvensson1
Copy link
Contributor Author

This PR currently experiences a bug associated with vertexLodMod because vertexLodMod code has been duplicated and misplaced and we have multiple uses of lodLevel with different semantics. I will need to refactor lodLevel as well before this PR can work properly.

@akortunov akortunov marked this pull request as draft November 5, 2021 10:42
@bosvensson1
Copy link
Contributor Author

bosvensson1 commented Nov 5, 2021

This PR should be complete now, but it could warrant some testing against different Groundcover view distances, vertex lod mod and lod factor values.

@bosvensson1 bosvensson1 marked this pull request as ready for review November 5, 2021 13:18
@akortunov
Copy link
Collaborator

While this PR seems to do its main purpose, I noticed terrain seams which do not present in the same place in master branch:

Screenshot_20211105_194443

[Terrain]
distant terrain = true

It is the hill directly to north from Fort Pelagiad. Also overall terrain details seem to be worse with this PR.

@AnyOldName3
Copy link
Member

The general idea here looks good to me. Obviously, we want to figure out the cause of the terrain degradation, though.

@bosvensson1
Copy link
Contributor Author

Thank you for the detailed testing. We appear to have experienced this degradation because my changes have revived some dead code in another module, but instead of being gifted additional functionality we have been gifted additional bugs in this case. I have now disabled this broken code and included some further cleanup. These latest changes should now work properly. It would be nice to test precisely the location on the above screenshot again if we have managed to save its coordinates.

@psi29a
Copy link
Member

psi29a commented Nov 8, 2021

Smoke testing and review of the effected area shows that seems issues have been resolved with a nice boost in performance. Thanks @bosvensson1 and @akortunov

screenshot057

@psi29a psi29a merged commit 5f1bf89 into OpenMW:master Nov 8, 2021
@bosvensson1
Copy link
Contributor Author

We appear to have slipped in a mistake at the last minute that was not noticed because I had just tested (0, 0) origin coordinates and @psi29a did not use a Groundcover setup. For those experiencing issues please try to delete the following line from groundcover.cpp:
box = osg::BoundingBox(box._min+worldCenter, box._max+worldCenter);

@psi29a
Copy link
Member

psi29a commented Nov 9, 2021

That was my fault, I was trying to match up to the screenshoot from @akortunov ; I have validated again with the change.
This time with groundcover...
screenshot058

I will push the fix.

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.

4 participants