-
Notifications
You must be signed in to change notification settings - Fork 102
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
base: master
Are you sure you want to change the base?
Particles upgrade #1636
Conversation
@@ -1,7 +1,7 @@ | |||
#version 430 core | |||
|
|||
uniform ivec2 arraySizes; | |||
uniform vec3 frameInfo; | |||
uniform vec3 frameInfo; // gs->frameNum, globalRendering->timeOffset, gu->modGameTime |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
f00362d
to
aea58ba
Compare
48ce93c
to
fe04e30
Compare
… to work with VA_TYPE_PROJ too
88d884b
to
828bf02
Compare
For me only to compare the changes