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

Particles upgrade #1636

Draft
wants to merge 72 commits into
base: master
Choose a base branch
from
Draft

Particles upgrade #1636

wants to merge 72 commits into from

Conversation

lhog
Copy link
Collaborator

@lhog lhog commented Aug 6, 2024

For me only to compare the changes

@@ -1,7 +1,7 @@
#version 430 core

uniform ivec2 arraySizes;
uniform vec3 frameInfo;
uniform vec3 frameInfo; // gs->frameNum, globalRendering->timeOffset, gu->modGameTime
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe a good opportunity to decouple particles from simframes? It would be good if they weren't bound to the discrete sim framerate

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Firstly, this is very work in progress with no due date or end goal. Potentially lots of classes will need to be reworked until I like the result and still this may not end up being merged because of extensive expectations from the GPU drivers.

Secondly, can you help and explain what is wrong with the simframe dependency?

Copy link
Collaborator

@sprunk sprunk Aug 23, 2024

Choose a reason for hiding this comment

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

Particles are unsynced so they don't need to be bound to frames, which would allow polishing graphics to arbitrarily high levels of detail. For example maybe you want a spark particle to live for 0.05s, and not 0.033s or 0.066s. Or say in the future a game could run with reduced sim FPS but still want smoothly turning particles (recall the old missile smoke trails that were extremely jagged if the missile turned quickly because they worked off movement calculations that applied at 4 FPS).

As a more general point, I think simframes as a measure of time should be avoided where possible (and seconds used instead) for reasons I described here. This philosophy mostly applies to things exposed to game devs, while this particle template shader is a very internal engine thing so it's not directly an issue (can always convert etc. and nobody would be able to tell), but my main worry is that if non-exposed internals keep using frames for timekeeping it would maintain some friction against making the parts I care about use seconds.

It's not a big deal though, so don't worry about it and ignore the suggestion if it's nontrivial.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, yes and no.

What you have here b615d16#diff-4e67dff21965b14807dab8fd197b27f0067694f286e0faf29d17dce6756bf20fR1084 is nothing else but the interpolated sim frame (or time, which is direct derivative). Essentially the same as frameInfo.x + frameInfo.y here https://github.com/beyond-all-reason/spring/blob/ef2224f5f281719b7667b36526d3a157637c1b19/cont/base/springcontent/shaders/GLSL/ParticleGeneratorTemplate.glsl#L4C14-L4C23

Now consider the update function here: https://github.com/beyond-all-reason/spring/blob/BAR105/rts/Sim/Projectiles/WeaponProjectiles/FireBallProjectile.cpp#L97-L114 The function changes quite a lot regarding the particle, spawns additional particles, etc. It's very hard to implement such special case into the system that handles 20 more particles classes (that are usually way simpler). I guess the point here is that it's nearly impossible to get rid of periodic CPU side updates for some classes (for others it's entirely possible).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, thanks for the explanation. Ignore the suggestion then.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think your suggestion is generally very on-point. Unfortunately some of the current classes are notable exceptions, we can't ignore.

@lhog lhog force-pushed the particles-upgrade branch 2 times, most recently from f00362d to aea58ba Compare August 26, 2024 18:23
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