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

Lily/ClonesPlus: re-re fix performance for "when I start as w/ set to ..." block #1763

Merged
merged 11 commits into from
Dec 28, 2024

Conversation

SharkPool-SP
Copy link
Collaborator

@SharkPool-SP SharkPool-SP commented Nov 27, 2024

Now it should work. It waits for variables to initialize

CubesterYT
CubesterYT previously approved these changes Nov 27, 2024
Copy link
Member

@CubesterYT CubesterYT left a comment

Choose a reason for hiding this comment

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

😔

@SharkPool-SP SharkPool-SP added the pr: change existing extension Pull requests that change an existing extension label Nov 27, 2024
@SharkPool-SP SharkPool-SP changed the title Lily/ClonesPlus.js -- let target initialize before starting hats Lily/ClonesPlus.js -- re-re fix performance for "when I start as..." block Nov 27, 2024
@CubesterYT CubesterYT dismissed their stale review November 30, 2024 03:07

I will wait a bit longer, until enough time has passed

@GarboMuffin
Copy link
Member

GarboMuffin commented Dec 25, 2024

Fails on this project

https://static.muffin.ink/extensions-1763.sb3

image

production:

image

this pull request:

image

Event does not get a chance to run at all

@GarboMuffin
Copy link
Member

GarboMuffin commented Dec 25, 2024

Anything that isn't an edge-activated hat will differ at least a little bit. That could break projects. But maybe worth doing since this is probably rare and that it's not running right after the vanilla block is arguably a bug. (if you follow this logic expected output would be when i start as clone then when i started as clone with then when timer then forever then end).

Here is where the vanilla event gets fired. You could do it somewhere around. Might not be able to do this spot exactly since the variable won't be set yet.

https://github.com/TurboWarp/scratch-vm/blob/b900a375fa1c4a9574835c308baf5f8f16fd6cd9/src/sprites/rendered-target.js#L177

@SharkPool-SP
Copy link
Collaborator Author

Anything that isn't an edge-activated hat will differ at least a little bit. That could break projects. But maybe worth doing since this is probably rare and that it's not running right after the vanilla block is arguably a bug. (if you follow this logic expected output would be when i start as clone then when i started as clone with then when timer then forever then end).

Here is where the vanilla event gets fired. You could do it somewhere around. Might not be able to do this spot exactly since the variable won't be set yet.

https://github.com/TurboWarp/scratch-vm/blob/b900a375fa1c4a9574835c308baf5f8f16fd6cd9/src/sprites/rendered-target.js#L177

fixed

@GarboMuffin GarboMuffin changed the title Lily/ClonesPlus.js -- re-re fix performance for "when I start as..." block Lily/ClonesPlus.js: re-re fix performance for "when I start as w/ set to ..." block Dec 27, 2024
@GarboMuffin GarboMuffin changed the title Lily/ClonesPlus.js: re-re fix performance for "when I start as w/ set to ..." block Lily/ClonesPlus: re-re fix performance for "when I start as w/ set to ..." block Dec 27, 2024
@GarboMuffin
Copy link
Member

btw the unrelated changes like Scratch.vm.runtime -> runtime probably didn't really need to be included

@GarboMuffin
Copy link
Member

!format

@SharkPool-SP
Copy link
Collaborator Author

btw the unrelated changes like Scratch.vm.runtime -> runtime probably didn't really need to be included

eh, ever so slightly faster + simpler

@GarboMuffin
Copy link
Member

at the cost of making the diffs bigger for no reason and increasing change of a typo causing a regression

@GarboMuffin
Copy link
Member

ok will use this pr to test more things related to the formatting workflow being broken

@GarboMuffin
Copy link
Member

!format

@GarboMuffin GarboMuffin merged commit dcdef7d into master Dec 28, 2024
3 checks passed
@GarboMuffin GarboMuffin deleted the SharkPool-SP-patch-3 branch December 28, 2024 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: change existing extension Pull requests that change an existing extension
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clones+ "when I start as clone with [ ] set to [ ]" causes very bad performance
4 participants