Further fixes for time transform snags; removed need for isTimeDependent; time-based builtin effects now using transforms #848
+300
−150
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…nd buffer and assignment logic Next up, prepare details from graphics.defineEffect()
…from Program Slight redesign of TimeTransform with caching (to synchronize multiple invocations with a frame) and slightly less heavyweight call site
Err, was calling Modulo for sine method, heh That said, time transform seems to work in basic test
Fixed error detection for positive number time transform inputs Relaxed amplitude positivity requirement
Maintenance Revert Test
My mistake adding back changes made
This reverts commit 0b5e1e9.
…far) isTimeDependent code disabled; now checking that shader objects were created and had either the TotalTime or DeltaTime uniform bound (or both, of course) ShaderResource's flag instead set when doing that Added a global flag as well, to say if ANY such flag was set (gets reset when read) Scene logic now checks the aforementioned flag and ensures the scene is invalidated if so (needed for flag to take hold, on first frame) Some commentary on these details The ApplyUniforms() logic is a bit different; the default time transform, if any, is done once up front each frame, so the default transformed time, plus the raw time, are on hand Any shader that specifies a transform passes it along via a command, and it will lazily be evaluated on the render side if TotalTime is bound On that note, no more attempts are done to check if transforms are dirty The code for the same is dummied out The built-ins that use time (marching ants, wobble, random) now use modulo-type transforms
Slight signature change for time transform Func type, plus changes for usage Removed some previously commented-out bits
…ead of GL backend
… absent :/ Now just setting the transform back to NULL after ApplyUniform() call
Shchvova
approved these changes
Sep 27, 2025
Contributor
Shchvova
left a comment
Shchvova
left a comment
There was a problem hiding this comment.
This is very nice. I'm worried about backwards compatibility.
It wouldn't affect existing code, would it?
Contributor
Author
|
Any incompatibility would indicate a bug, as far as I can tell. It shouldn't stomp on any existing expected behavior. Also, there were only those three builtins, and I confirmed they were using the transforms. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
(This probably needs to wait for word that the Vulkan backend behaves right, beyond just compiling, but otherwise feels good.)
In Discord it came up that time transforms still have a few issues. Assigning one to a shader was broken. Adding isTimeDependent to a shader when it wasn't used could cause some things to break, as well.
It was also mentioned that the marching ants builtin (a time-based effect) got worse over time.
The broken shader assignment was something silly: doing a
lua_isstring(), when the string in question had already been popped off the stack, rather than check for the non-NULLresult.The fixed behavior is used by the time-based builtins, described below.
isTimeDependent:
Once a scene has been drawn, Solar will leave it alone if nothing changes, i.e. no new objects; objects being moved or rotated; etc. This is mostly the right behavior, but time-based effects do need to be redrawn. The isTimeDependent flag, passed to shader definitions, was used to convey this information.
The shader author needed to remember to add this flag. And to be careful to remove it / not add it otherwise, apparently.
The time transforms could also use this information as a hint about when they needed to be called. I was attempting some caching here (from #511), and it apparently blew up if you wrongly provided the flag.
This PR ignores isTimeDependent entirely and discovers the dependency in a different way.
We still query
ShaderResource::UsesTime(), but the state itself is being found automatically: if theCoronaTotalTimeuniform is bound, following aGLProgram::Create(), the flag is set. (Any variant, e.g. # of masks or distorted rects, can be responsible, and the flag is taken as normative for all of them.) This is within the firstGLProgram::Bind()of the variant; there's an untestedDEFERpath too, if anything ever wants to use that.There are corresponding Vulkan backend parallels, too.
The scene will have already been updated by the time the
Create()is performed, and thus be unaware of any new time dependency. If anyShaderResourcehad its flag set, the scene is therefore invalidated; it will pick up on anything new the next time around.Some commentary about this may be found in Rtt_Scene.cpp.
The default time transform is applied to the raw total time when the scene begins rendering, and saved in a value. This is used by shaders without custom transforms; otherwise the raw time is provided, so that it can be transformed if necessary. In the latter case the
CoronaTotalTimeuniform's update is couched between bind and unbind commands for the transform.Other than the default up front, no attempt is now made at transform caching, or even checking that the shader uses time, while issuing commands. If the time uniform is actually bound, any transform is resolved when the corresponding command is executed.
There are three time-based builtins: generator.marchingAnts, filter.wobble, and generator.random.
Each was unbounded and so would eventually suffer some artifacts. However, all three are "modulo"-friendly cases and were given such a transform.
Marching ants is a repeating pattern. It moves at a certain speed (16), and once it's covered two bars' worth of distance (off and on, so 6 + 6), we have an identical-looking effect. Thus its range is (6 + 6) / 16 = .75 seconds.
Wobble uses time inside a sine. One period will have elapsed when 3 * t = 2 * pi, so our range is 2 / 3 * pi.
Random was just doing a
fract(), so there the range is 1.These numbers are found in the effects themselves. I added similar commentary in there. Old code is still present, commented out; forgot about it during cleanup, but I guess it's fine for comparison purposes.
I was testing with this:
fx_tests.zip
I had it set up to try the time-based builtins as well as copies of their code, with minor modifications in how they calculated time, and now without using isTimeDependent at all. (Those versions might be a little odd themselves over a certain duration, since they should now use the default "pingpong" behavior, but were used to manually check the modulo ranges and the code still reflects that.)
Some of the
if trues and such were for quickly disabling certain effects to make sure we didn't still have isTimeDependent at work, as well as to verify that a delay between shader creation and bind would still behave right. (This is currently in place and the reason the effects take a few moments to start.)