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

Apply fog in fragment shader in terrain_raster program #12423

Merged
merged 3 commits into from
Nov 30, 2022

Conversation

endanke
Copy link
Contributor

@endanke endanke commented Nov 29, 2022

In GL-Native we do some optimization on the terrain grid by rendering less vertices when the exaggeration is set to zero. (This is to be ported to GL-JS.) In those cases, the fog seems to be applied inconsistently across different zoom level tiles, which is more visible on darker areas.

If we apply the fog in the fragment shader (similarly to our layer-specific implementations) it is rendered correctly.

In render-tests/fog/terrain/equal-range we can see a section of a terrain which is close to the camera, where previously the fog was applied in a sawtooth like pattern. This change fixes it too.

Before (with a low-poly terrain):

Screenshot 2022-11-29 at 7 59 05

After:

Screenshot 2022-11-29 at 8 01 07

Launch Checklist

  • briefly describe the changes in this PR
  • include before/after visuals or gifs if this PR includes visual changes
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page
  • tagged @mapbox/map-design-team @mapbox/static-apis if this PR includes style spec API or visual changes
  • tagged @mapbox/gl-native if this PR includes shader changes or needs a native port
  • apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
  • add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog>Fixes fog rendering on low-poly terrain grid</changelog>

@endanke endanke requested a review from a team as a code owner November 29, 2022 08:21
@endanke endanke force-pushed the endanke/fix-fog-zero-exaggeration branch from f069e43 to 7bde827 Compare November 29, 2022 08:56
Copy link
Contributor

@karimnaaji karimnaaji left a comment

Choose a reason for hiding this comment

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

Linking original PRs and profiling related to the evaluation being done in the vertex shader (cc @rreusser):

In the initial testing done, this had a measurable performance difference (of roughly 1ms of gpu time, with the tradeoff of a minor visual difference due to the evaluation being done in the vertex shader).

It'd be worth profiling this PR on mobile to understand the performance impact. If there's still any measurable difference and this leads to issues with 0-elevation terrain, maybe there's an alternative solution that could retain the performance gain for elevated terrain while still fixing the visual issue.

Otherwise the change looks good to me.

@endanke
Copy link
Contributor Author

endanke commented Nov 29, 2022

Thanks for the context @karimnaaji !
I think I could add a new define to use a different codepath when the exaggeration is zero. This would be useful in other cases as well to return earlier when we know that the elevation would be flat.

@karimnaaji
Copy link
Contributor

@endanke yes that's an option! But if you don't notice any performance difference in your new measurements, let's keep it simple as you have it here. It's just worth understanding the performance impact before merging as it may offload a lot of computations to the fragment shader and may be beneficial for gpu-bound situations on mobile.

@endanke
Copy link
Contributor Author

endanke commented Nov 29, 2022

Definitely, I'll run some measurements before merging.

@endanke
Copy link
Contributor Author

endanke commented Nov 30, 2022

I've run a couple of tests on a Pixel 2 and a Nexus 5 devices, in a setup where the camera moves across different areas with terrain. I've re-run the same sequence 3x10 times, and both the median frame time and the dropped frames are pretty much the same between the before and after cases. (Considering the variance of the runs the difference is within the margin of error.) These tests were run in GL-Native, so there might be still performance changes in GL-JS due to this change.

However, I've also run a couple of tests to measure some optimizations for the zero exaggeration case. We could add a flag to return earlier with a zero in the elevation function and it seems to reduce the frame times around 5%. The same flag can be used here to use the previous implementation if the exaggeration is non-zero.

@endanke endanke force-pushed the endanke/fix-fog-zero-exaggeration branch 2 times, most recently from 16da14d to 43c3236 Compare November 30, 2022 14:26
@endanke endanke force-pushed the endanke/fix-fog-zero-exaggeration branch from 43c3236 to 5914011 Compare November 30, 2022 14:37
@endanke endanke merged commit 31f03a6 into main Nov 30, 2022
@endanke endanke deleted the endanke/fix-fog-zero-exaggeration branch November 30, 2022 14:49
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.

2 participants