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

Jank-free rendering & shared tickers (WIP) #10

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

JonDum
Copy link

@JonDum JonDum commented Sep 13, 2018

I added a setValue function that acts as sugar
for updating config and starting. This is somewhat akin
to setEndValue in rebound. Name of method is totally
up for debate.

Second, I added a raf config option (again, name up for
debate) that stops _step() from registering its own
requestAnimationFrames. With raf: false all spring
advancing must be done manually.

That said, I mangled the living hell out of _step
and advanceSpringToTime in order to get this to work.

It needs some cleaning up. Perhaps _step becomes
what _advanceSpring is now? Usually in GL land
step/tick functions are called by your main draw loop
to do whatever time-based advancing they need to do, so there'd
be some parity to existing convetions that way.

Finally, check out the squares demo I added (again it's messy).
If you watch the performance monitor heap allocation remains
constant and there's only a single raf frame 👍

Inner function should be free of memory allocation
Also mangles _step and `advancedToTime` pretty bad.
this._config.toValue = value;
this._config.initialVelocity = this._currentVelocity;
this.start();
this._advanceSpringToTime(undefined, true);
Copy link
Author

Choose a reason for hiding this comment

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

I definitely don't like this (undefined, true). Perhaps it'd be best to move the raf loop into start/stop and the spring logic in step?

</head>
<body>
<div id="app"></div>
<script src="./index.tsx"></script>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure why you're using .tsx here. You aren't using TypeScript or JSX, and even if you were, they'd need to be converted to JavaScript before being included in a page.

Copy link
Author

Choose a reason for hiding this comment

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

The dev system transpiles it to a js when running yarn dev. I just copied from the other demo.

...this._config,
...baseConfig,
...updatedConfig
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure why you felt the need to change this. Was there a problem, or do you just prefer the iterative style?

Copy link
Author

Choose a reason for hiding this comment

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

#9

this._config.toValue = value;
this._config.initialVelocity = this._currentVelocity;
this.start();
this._advanceSpringToTime(undefined, true);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this just sugar over .updateConfig({ toValue: })?

Copy link
Author

@JonDum JonDum Sep 17, 2018

Choose a reason for hiding this comment

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

Yes. To avoid the object literal and extra steps updating the config when you only need to update the value (which is going to be most use cases) instead of the whole config.

@appsforartists
Copy link
Collaborator

Thanks for contributing!

I'd like to see more context. Were there issues you were running into that led to these changes? Can we separate them out into individual pieces (e.g. setValue sugar, skipping raf, using loops instead of literals) so we can discuss them independently?

Personally, I'm skeptical of changes that only have theoretical benefits or suit a personal style. If you can say "it drops frames when there are more than 4 springs running on this class of device," I'm more amenable to changes than "this is theoretically faster if you have eleventy million springs instantiated per frame."

I don't mean to be a skeptic or pedant, but I would like to understand the motivation for changes before commenting on or approving them.

Also, since this is a microlibrary, I'm especially conservative about API sugar. I don't see much benefit of setValue(toValue) vs updateConfig({ toValue }), but replacing updateConfig with setters will impact file size.

@JonDum
Copy link
Author

JonDum commented Sep 17, 2018

#9
#8

I've been using my fork for a while now and have significantly less jank. My use case is with webgl drawing as many thousands of verts as I can as smoothly as I can. Every major/minor GC results in a noticeable hiccup and drop from 60fps, so this pull is about providing pathways that avoid all memory allocation so gc's don't even happen.

Use class references to avoid unnecessary closures on draw calls
@JonDum
Copy link
Author

JonDum commented Sep 17, 2018

Charts are nice, so here's a couple.

Here's the js heap size using wobble normally (letting it make its own requestAnimationFrames and using updateConfig({toValue: value})

screen shot 2018-09-17 at 12 18 42 pm

Edit: forgot that the above is also on my PR branch, so this is probably a bit better still than skevy/wobble/develop because of the changes I made in updateConfig to reduce allocations

And here's the exact same demo using {raf: false}, setValue(value), and tick/_advanceSpringToTime:

screen shot 2018-09-17 at 12 09 02 pm

Half the cpu usage, half the memory usage and no GCs. 👍

@JonDum
Copy link
Author

JonDum commented Sep 28, 2018

Any thoughts?

@nickschot
Copy link

Any hopes of getting this PR merged? I know its been a long time 😅

@JonDum
Copy link
Author

JonDum commented Apr 5, 2020

Author doesn't really seem interested. Might as well use my fork if performance is important to you.

@nickschot
Copy link

That is indeed what I was considering in case there was no response! Thanks 🙂

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.

3 participants