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 particle effect randomness #388

Merged
merged 5 commits into from
Sep 15, 2021
Merged

Fix particle effect randomness #388

merged 5 commits into from
Sep 15, 2021

Conversation

iche033
Copy link
Contributor

@iche033 iche033 commented Aug 24, 2021

Signed-off-by: Ian Chen ichen@osrfoundation.org

🦟 Bug fix

Summary

The randomness algorithm in the particle effect for has issue when time (which is used to add randomness) becomes large in the shaders. The fix is to just use the fractional part of the time value for adding randomness.

To Test

To reproduce the problem, one way to test this is to artificially increase the time value in the shaders. You can do this by applying the patch below to this branch to simulate the old behavior when time is large:

diff --git a/ogre2/src/media/materials/programs/depth_camera_fs.glsl b/ogre2/src/media/materials/programs/depth_camera_fs.glsl
index 0d3109d5..a22bd4b3 100644
--- a/ogre2/src/media/materials/programs/depth_camera_fs.glsl
+++ b/ogre2/src/media/materials/programs/depth_camera_fs.glsl
@@ -107,7 +107,7 @@ void main()
   if (particle.x > 0 && particleDepth < fDepth)
   {
     // apply scatter effect so that only some of the smoke pixels are visible
-    float r = rand(inPs.uv0 + vec2(rnd, rnd));
+    float r = rand(inPs.uv0 + vec2(rnd + 1e5, rnd + 1e5));
     if (r < particleScatterRatio)
     {
       // set point to 3d pos of particle pixel

Then run this example world:
https://gist.github.com/iche033/bcd3b7d3f4874e1e707e392d6dbb0aa0

The patch adds 100000 seconds (27.x hours) to time and you'll see bands across the rgbd camera :

rgbd_particles_large_t

The changes in this PR makes sure the random number never exceeds 1 and the noise pattern should stay distributed across the image:
rgbd_particles

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge

Signed-off-by: Ian Chen <ichen@osrfoundation.org>
Signed-off-by: Ian Chen <ichen@osrfoundation.org>
@github-actions github-actions bot added the 🔮 dome Ignition Dome label Aug 24, 2021
@codecov
Copy link

codecov bot commented Aug 24, 2021

Codecov Report

Merging #388 (caacd56) into ign-rendering4 (a3679f4) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head caacd56 differs from pull request most recent head 9a9a8dc. Consider uploading reports for the commit 9a9a8dc to get more accurate results
Impacted file tree graph

@@               Coverage Diff               @@
##           ign-rendering4     #388   +/-   ##
===============================================
  Coverage           55.91%   55.91%           
===============================================
  Files                 147      147           
  Lines               13981    13983    +2     
===============================================
+ Hits                 7817     7819    +2     
  Misses               6164     6164           
Impacted Files Coverage Δ
ogre2/src/Ogre2ParticleNoiseListener.cc 89.47% <100.00%> (+0.38%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a3679f4...9a9a8dc. Read the comment docs.

@darksylinc
Copy link
Contributor

darksylinc commented Aug 29, 2021

I knew there was something amiss with that line!

Your solution is a temporary workaround though. Once time becomes large enough, the fractional part becomes always 0 (and before that, it will always be 0 and 0.5, and before that...).

It's easier to mentally visualize it with half floating point format

A better solution would be to fix it from C++ side of things.

If you're using Ogre's auto param, IIRC there was an option to make it periodic.

Update: it's time_0_x 1
Update 2 ugh Ogre makes the same mistake of doing mod in floating point

Signed-off-by: Ian Chen <ichen@osrfoundation.org>
@iche033
Copy link
Contributor Author

iche033 commented Aug 30, 2021

A better solution would be to fix it from C++ side of things.

yeah you're right, once time becomes too large, the randomness goes away. I took the C++ approach and passed a random number to the shaders. 2690ca2

@iche033 iche033 merged commit cda2c27 into ign-rendering4 Sep 15, 2021
@iche033 iche033 deleted the particle_rand branch September 15, 2021 18:35
@peci1 peci1 mentioned this pull request Nov 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔮 dome Ignition Dome
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants