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 precision issues in grid #595

Merged
merged 1 commit into from
Jul 24, 2020
Merged

Fix precision issues in grid #595

merged 1 commit into from
Jul 24, 2020

Conversation

daleeidd
Copy link
Collaborator

@daleeidd daleeidd commented Jul 21, 2020

I have been seeing gaps in the ocean tiles at particular coordinates. It appears to be a precision issue in the grid. This is not screen resolution dependent so you should be able to reproduce with provided example scene.

1_Broke
Without fix

1_Fixed
With fix

I am not sure if this is just my machine or not. I would think I would have noticed this when developing the black point fade fix which was developed on different machines. I had a look at the LOD fade and it didn't have any affect on this issue.

It appears when either the X or Z coordinates is nearly divisible by 8 (so 7.999997 or 15.999997). The gaps will appear in different locations depending how large the whole number is.

Geometry Down Sample Factor has an affect but only a value of 8 clears it without the fix.

Increasing 2.0 by 0.000001 is the smallest number that fixes the issue. Going larger will start to spread the grid apart.

I will fix up the commit if this is indeed an issue on other machines. I experienced the issue on MacOS and Windows (both on MacBook Pro), but only tested the fix on MacOS. I looked back and this issue occurs for me back in commits from 2019. So I am a bit suspicious of my computer. Occurs on HDRP too. I haven't tested URP but I guess it would be there too.

Testing

Open up the PrecisionIssuesExample scene. Enter play mode and the gaps should be there. Enable the fix on the ocean material to have it fixed.

@daleeidd daleeidd added the Bug label Jul 21, 2020
@huwb
Copy link
Contributor

huwb commented Jul 22, 2020

Ohh increasing the grid size by epsilon... i think thats a reasonable fix, yes. Well found.

Increasing 2.0 by 0.000001 is the smallest number that fixes the issue. Going larger will start to spread the grid apart

So this this a safe value? It sounds like you found the lower bound, and theres also an upper bound before the grid starts to be spread apart, so this value is comfortably in the sweet spot?

Nice one

@daleeidd daleeidd marked this pull request as ready for review July 24, 2020 05:37
@daleeidd
Copy link
Collaborator Author

@huwb I ended up finding an even smaller value. I noticed that the original produced a very fine separation (appeared as sparsely dotted line) at certain displacements when working on the underwater post processing and inspecting the underside up close (it's great at highlighting gaps).

Old and new value

0.000001
0.00000012

Commit is ready to be reviewed

Copy link
Contributor

@huwb huwb left a comment

Choose a reason for hiding this comment

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

Probably a complete tangent but are both of these functions used? :)

Change itself looks good to me

@daleeidd
Copy link
Collaborator Author

They are for now. The one in the OceanVertHelpers.hlsl files is the most recent one which we use in shader graph. The other is for the written shader. Just need to continue refactoring/unifying to get rid of the old ones. OceanHelpers.hlsl and OceanLODData.hlsl files are old and will be removed once we transfer fully to the newer functions.

Thanks. I'll merge.

@daleeidd daleeidd added this to the Crest 4.5 milestone Jul 24, 2020
@daleeidd daleeidd merged commit 600acd9 into master Jul 24, 2020
@daleeidd
Copy link
Collaborator Author

daleeidd commented Sep 3, 2020

Just adding an image of the fine separation mentioned above caused by the epsilon value. I noticed it recently on inspecting the underside of the ocean surface closely. It is pretty rare. I think this is better than the original issue.

Precision Issue

I tried to see if I could find a value to solve both. The smallest value (any lower rounds) I could find that solved the larger gaps unfortunately still caused the fine gaps to occur. The smallest value:

2.00000011920929

The only solution I can think of is something like _CameraPosition.x % 8 > 7.9 || _CameraPosition.z % 8 > 7.9 ? 2.00000012 : 2.0. Since we know the large gaps only occurs around multiples of 8 but the fine gaps do not. It could be driven with scripting to save on GPU costs.

@huwb
Copy link
Contributor

huwb commented Sep 8, 2020

That's annoying about those pesky gaps.

It might be worth just scaling the vert position up a tiny bit to make the role sightly bigger at the beginning of the vert shader. It feels like expanding the tile should combat the gap issues? I can propose something.

@huwb
Copy link
Contributor

huwb commented Sep 8, 2020

Does having something like

v.vertex.xyz *= 1.001;

At the beginning of the vertex shader help at all?

@daleeidd
Copy link
Collaborator Author

daleeidd commented Sep 8, 2020

Thanks. That does work. That value caused a mismatch with blending at the edges, but something like 1.000009 solved the problem without it.

I managed to find the smallest values that would solve the problem for both:

v.vertex.xyz *= 1.00000101327897;
const float GRID_SIZE_2 = 2.00000011920929 * gridSize;

But we could get away with:

v.vertex.xyz *= 1.00001;
const float GRID_SIZE_2 = 2.000001 * gridSize;

@huwb
Copy link
Contributor

huwb commented Sep 9, 2020

Sounds promising!

I was hoping that overlap between tiles would be fine. Is there a visible issue when tiles overlap?

And we still need the epsilon on grid_size_2 (just to check)?

@daleeidd
Copy link
Collaborator Author

daleeidd commented Sep 9, 2020

I was hoping that overlap between tiles would be fine. Is there a visible issue when tiles overlap?

There was an issue. But it went away when scaled by a smaller number. (1.00001 instead of 1.001)

And we still need the epsilon on grid_size_2 (just to check)?

We do.

@huwb
Copy link
Contributor

huwb commented Sep 9, 2020

Ok lets roll with it then!

@daleeidd
Copy link
Collaborator Author

daleeidd commented Sep 9, 2020

Thanks. Done with 7633368

@daleeidd daleeidd deleted the fix/grid-precision-gaps branch April 1, 2021 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants