-
Notifications
You must be signed in to change notification settings - Fork 125
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
Add createSpring primitive. #629
Add createSpring primitive. #629
Conversation
|
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.
Currently it allows you to pass a string as an input (or an array of strings, object with string values etc), but you could use a type like this to provide some validation
type SpringTargetPrimitive = number | Date;
type SpringTarget =
| SpringTargetPrimitive
| { [key: string]: SpringTargetPrimitive | SpringTarget }
| SpringTargetPrimitive[]
| SpringTarget[];
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.
That's a good suggestion. Was also considering this. I just made it close to Svelte's implementation which was more loose with types.
But under the hood it does actually only implement those specific primitives: Date, Number, Array<Date | number>, and { [key: any]: Date | number> }.
Will definitely add it here. Thanks!
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.
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.
Just a side note but the "validation" will not catch this:
const [spring, setter] = createSpring({foo: [1,2,3]} as {foo: number[]} | number)
setter(101)
// runtime error: Cannot read properties of undefined (reading '0')
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.
And it seems to break with readonly number[]
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.
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.
Also in source code there is a current_value == null
check, which I think is impossible with the validation.
Update: I also figure of adding const springValue = createDerivedSpring(signal, opts) Will add this tonight. |
18ee12d
to
45d16ec
Compare
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.
Thanks, this is a very nice addition.
Hi @thetarnav just pushed the latest changes based on your review. Thanks so much for the very thorough comments on it. I haven't done the tests related to timeouts yet because as you said, timeouts are kind of my nightmare too lol. I'll take a gander at it again soon (maybe tomorrow or in a few hours). If you want to try playing around with it (with the same example video above). Try running this: https://github.com/blankeos/spring-solid-test (currently I just copy paste the |
82e0965
to
0ada2ae
Compare
I don't necessarly like that |
I tried to inline the tasks thing from svelte to the primitive. The tests pass and it works as it should in my eyes, but could you double check @Blankeos? |
} | ||
|
||
if (raf_id === 0) { | ||
time_last = performance.now() |
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 it should be something like Number(document.timeline.currentTime || performance.now())
ala MDN
Otherwise, in cases where document.timeline.currentTime
is available, the first frame could have a negative timeDelta
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.
hmm youre right
I'm consistently getting negative values with performance.now
.
But with document.timeline.currentTime
it's always zero.
Which makes me think that maybe this would be better:
requestAnimationFrame(prev_time => {
function frame(time) {
let delta = time-prev_time
prev_time = time
requestAnimationFrame(frame)
}
requestAnimationFrame(frame)
})
// or
let prev_time = Infinity
function frame(time) {
let delta = Math.max(0, time-prev_time)
prev_time = time
requestAnimationFrame(frame)
}
requestAnimationFrame(frame)
Since the first iteration will be ran with 0
delta anyway which won't progress the animation at all, so it could be skipped.
Instead of relying on document.timeline.currentTime
, which may be and may not be there, and falling back to performance.now
which is wrong.
} | ||
|
||
if (opts.soft) { | ||
inv_mass_recovery_rate = 1 / (typeof opts.soft === "number" ? opts.soft * 60 : 30); |
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.
This hard-codes an assumed 60 fps into the inverse mass recovery feature. On different hardware, opts.soft
will behave differently (inv_mass will return to 1 more quickly with higher framerate) and more importantly the velocity
will be incorrect.
I'm not sure the best fix for fps-independence here, but in my own adaptation I have gone for tracking the fps as Math.max(fps, 1000 / (time - timeLast))
in the frame
function above
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.
Turns out tracking fps this way is very flawed. At best it gives an inaccurate value (consistently getting ~180 fps when I should really be getting 166) and at worst is can go to extreme values and be very inconsistent.
I can look into frame-independent impls unless somebody else wants to jump in with a solution
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.
My intuition would be to keep the recovery rate as a constant, only depending on opts.soft
.
And use time delta in inv_mass = Math.min(inv_mass + inv_mass_recovery_rate * time_delta, 1)
But I'm curious about other directions.
I'll try to add a test for framerate-independence since we are mocking the raf anyway.
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 was able to fix the inv_mass_recovery_rate
to actually recover in the correct amount of time, but couldn't accomplish the same for the tick
function
solid-primitives/packages/spring/src/index.ts
Lines 152 to 159 in eeff964
const tick = (last: T, current: T, target: T): any => { | |
if (typeof current === "number" || is_date(current)) { | |
const delta = +target - +current; | |
const velocity = (+current - +last) / (time_delta || 1 / 60); // guard div by 0 | |
const spring = stiffness * delta; | |
const damper = damping * velocity; | |
const acceleration = (spring - damper) * inv_mass; | |
const d = (velocity + acceleration) * time_delta; |
The frame-dependent
time_delta
is baked into this implementation in tick
. It doesn't quite cancel out even though d
is multipled by time_delta
in the end either, but I'm not totally sure what is supposed to be going on instead.
https://github.com/pqml/spring/blob/master/lib/Spring.js - this looks promising, it is based on picking a timestep, checking how many of those timesteps have passed since the last frame, and solving for the position that many times
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.
Makes sense. I'm using the same technique in my force-graph implementation—which has the same problem that it cannot be simply "multiplied by delta_time" to make the simulation frame-independent—and it works pretty well: https://github.com/thetarnav/force-graph/blob/main/index.mjs#L86-L98
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 I’m going to merge this in the current state as it’s already useful and matches sveltes version.
The implementation could be improved to be frame-independent in future PRs as it shouldn’t change the current interface.
Awesome work guys! Couldn't really check the past week, it's been a very busy month for me. Thanks for adding in your insights @thetarnav @rcoopr! 🦾🥳 |
Summary
createSpring
primitive. Inspired and Forked from https://svelte.dev/docs/svelte-motion#springtween
primitive.Example Usage:
Witness, the springiness:
solidjs.spring.primitive.-.demo.mp4
Solid.CreateSpring.Demo.mp4
Todos: