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/Video: Add new 'frame of video (my video) at (0) seconds' block #1536

Closed
wants to merge 3 commits into from

Conversation

LSPECTRONIZTAR
Copy link

@LSPECTRONIZTAR LSPECTRONIZTAR commented Jun 11, 2024

New block for the Lily/Video extension that adds a block 'frame of video (my video) at (0) seconds' where it returns the data:URI of the frame of the video at a given time in the video. Can be useful for trying to draw videos in 3D space (using Pen+) making video editing projects similar to CapCut or PowerDirector.

Let me just make it clear that @SharkPool-SP made this extension, not me (the only JavaScript I know is JSON tbh lol). He gave me permission: #1536 (comment)

This is my first PR i need this so bad

@AshimeeAlt
Copy link
Contributor

maybe move the comment saying sp made the block above the function or the block json

@LSPECTRONIZTAR
Copy link
Author

maybe move the comment saying sp made the block above the function or the block json

Will that fix the checks? I know this ain't getting merged without all successful checks

@LSPECTRONIZTAR
Copy link
Author

maybe move the comment saying sp made the block above the function or the block json

IMG_0089
like this?

@AshimeeAlt
Copy link
Contributor

maybe move the comment saying sp made the block above the function or the block json

IMG_0089 like this?

yes but its not going to fix the prettier check, it will be fixed when merged

@LSPECTRONIZTAR
Copy link
Author

maybe move the comment saying sp made the block above the function or the block json

IMG_0089 like this?

yes but its not going to fix the prettier check, it will be fixed when merged

I don't think it's getting merged unless all checks are successful but ok

@LSPECTRONIZTAR LSPECTRONIZTAR changed the title Lily/Video.js: Add new 'frame of video (my video) at (0) seconds' block Lily/Video: Add new 'frame of video (my video) at (0) seconds' block Jun 11, 2024
@SharkPool-SP
Copy link
Contributor

Just in case Garbo sees this, I did give permission for him to use my modified code and make the pr.

@Drago-Cuven
Copy link

pleeeease!?

@LSPECTRONIZTAR
Copy link
Author

PLEASE?!?

@Drago-Cuven
Copy link

Drago-Cuven commented Jul 2, 2024

PLEASE?!?

I srsly want this merged.

@LSPECTRONIZTAR
Copy link
Author

PLEASE?!?

I srsly want this either merged.

ME TOO

@LSPECTRONIZTAR
Copy link
Author

I NEED IT FOR OSU!3D

@Drago-Cuven
Copy link

I NEED IT FOR OSU!3D

ooh yes.

i'm planning on doing a 3d game with VR support eventually too if I get the chance to.

@GarboMuffin
Copy link
Member

you can of course use it before it gets merged by just copying and pasting the code for the extension and adding it as a custom one

what bothers me about this implementation is that although its "get frame" which would imply no side effects, it does in fact have the side effect of seeking the video around

@GarboMuffin
Copy link
Member

GarboMuffin commented Jul 2, 2024

and there is a code path inside the Promise that never calls resolve or reject (so the block can get stuck forever), and you don't handle when the browser doesn't give you a 2d rendering context (some safari versions are quite picky about this), and using a promise forces a 1 frame delay every time it runs, even if you just want to get the image that the video is currently showing

@LSPECTRONIZTAR
Copy link
Author

I NEED IT FOR OSU!3D

ooh yes.

i'm planning on doing a 3d game with VR support eventually too if I get the chance to.

yoooo me too

@LSPECTRONIZTAR
Copy link
Author

and there is a code path inside the Promise that never calls resolve or reject (so the block can get stuck forever), and you don't handle when the browser doesn't give you a 2d rendering context (some safari versions are quite picky about this), and using a promise forces a 1 frame delay every time it runs, even if you just want to get the image that the video is currently showing

i don't get it
but i'll tell sharkpool about it anyway

@GarboMuffin
Copy link
Member

The person who wrote the code should be sending the pull request...

@LSPECTRONIZTAR
Copy link
Author

LSPECTRONIZTAR commented Jul 3, 2024

The person who wrote the code should be sending the pull request...

well he told me to do it if i wanted it that badly

@GarboMuffin
Copy link
Member

GarboMuffin commented Jul 4, 2024

Shark pool telling other people to submit code your code for you wastes your time, my time, and the person who submitted its time.

@SharkPool-SP
Copy link
Contributor

Shark pool telling other people to submit code your code for you wastes your time, my time, and the person who submitted its time.

I told him to do it if he wants. I personally thought it was fine lmao. Next time ill know what to do

@LSPECTRONIZTAR
Copy link
Author

and there is a code path inside the Promise that never calls resolve or reject (so the block can get stuck forever), and you don't handle when the browser doesn't give you a 2d rendering context (some safari versions are quite picky about this), and using a promise forces a 1 frame delay every time it runs, even if you just want to get the image that the video is currently showing

WAKE UP I FIXED I- uhhh I mean SHARKPOOL FIXED IT
please merge this 🥺🥺🥺🥺🥺🥺🥺🥺🙏🙏🙏🙏🙏🙏🙏

@SharkPool-SP
Copy link
Contributor

#1595

@LSPECTRONIZTAR
Copy link
Author

#1595

Does it have the fixed video frame thing

@SharkPool-SP
Copy link
Contributor

Yes

@LSPECTRONIZTAR
Copy link
Author

Yes

If GarboMuffin can't merge this one he'd better merge that one then :)

@SharkPool-SP
Copy link
Contributor

Well he kinda has to, it also fixes a bug with another block

@LilyMakesThings
Copy link
Contributor

Yes

If GarboMuffin can't merge this one he'd better merge that one then :)

close this one

@LSPECTRONIZTAR
Copy link
Author

Yes

If GarboMuffin can't merge this one he'd better merge that one then :)

close this one

Exactly what I was gonna do, thanks for the reminder

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.

6 participants