Skip to content

Conversation

@sophiedeziel
Copy link
Collaborator

@sophiedeziel sophiedeziel commented Oct 13, 2024

With every re-render, geometries are recalculated even though paths have not changed. Memoizing the geometry building method helps with that.

It has no perceptible performance impact the initial render, even with progressive rendering. However, the next renders are significantly faster and we can notice.

There are downsides of memoizing as it requires more memory. It should be an option exposed, because it really depends on the use case. If the same model has to be re-rendered multiple times, memoization is great. If only one render is needed per gcode file, cache should be off. I am not sure what the default should be.

What do you think? Should this PR include the option? What should be the default?

@sophiedeziel sophiedeziel changed the base branch from develop to v3.x October 13, 2024 16:27
@github-actions
Copy link

github-actions bot commented Oct 13, 2024

Visit the preview URL for this PR (updated for commit a4921d6):

https://gcode-preview--pr222-cache-geometries-t6ymg497.web.app

(expires Fri, 15 Nov 2024 01:41:21 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 59bd114ae4847b32c2bba0b68620b9069a3e3531

@sophiedeziel sophiedeziel changed the title Geometries are not re-calculated at each render Don't recalculate Geometries at each render Oct 13, 2024
@sophiedeziel sophiedeziel force-pushed the v3.x branch 2 times, most recently from 10dc1f2 to 0b06ec9 Compare October 13, 2024 23:42
@sophiedeziel sophiedeziel force-pushed the cache-geometries branch 2 times, most recently from e834811 to 79d4701 Compare October 14, 2024 00:39
@sophiedeziel
Copy link
Collaborator Author

I took time to think about it. This is a net new feature that does not come without risk.

This idea seemed easy an quick, but I'm realizing it will be a distraction and a slow down to get v3.x in a mergable state. Changing any setting that affects the geometry (extrusionWidth and lineHeight) requires cache invalidation and we should not embark in that right now.

I suggest to bring that optimization back later on, as an experimental feature that is off by default. The risk of introducing bugs or having unintended effects is pretty high.

Closing this PR with the intention to revisit the idea later if we want to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants