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

[WIP] Video extension #934

Closed
wants to merge 33 commits into from
Closed

[WIP] Video extension #934

wants to merge 33 commits into from

Conversation

Bouh
Copy link
Collaborator

@Bouh Bouh commented Feb 14, 2019

The declarations of the events are made and all events work

I need help for :

  • Catch error if video is not found
  • Implement ressource manager (I've hardcoded all paths)
  • Review of code, and optimizing

You can see with this Keywords my notes or todos :
//FIXME
//TODO
//NOTE

Here an video for run this project with all events video
And this icon missing_video24 if the file is not found : missing_video24.png

@4ian
Copy link
Owner

4ian commented Feb 15, 2019

I'll check this this week-end or before if possible :)

@4ian
Copy link
Owner

4ian commented Feb 16, 2019

I've done heavy changes in master branch to support translation. See if you can merge master in this branch. This should not conflict with the changes you've made here.

@Bouh
Copy link
Collaborator Author

Bouh commented Feb 17, 2019

I've changed t("") to _("") and merge the master.
I've missing something or not ? Because all my expressions action and conditions have an error :

Error: "Cannot find an expression with this name: getDuration Double check that you've not made any typo in the name." in: "MonObjetVideo.getDuration()" (number) libGD.js:20

image

image

@4ian 4ian changed the title [WIP] Video extention [WIP] Video extension Feb 18, 2019
@4ian
Copy link
Owner

4ian commented Feb 18, 2019

I've missing something or not ? Because all my expressions action and conditions have an error :

Yes, t and _ are functions (not magic macro like in C++).. so if you rename a function name, you must also rename its name in the arguments of the createExtension function ;)
I fixed that for you. I did not have the time to test, so I hope it's ok.

If you see "emsc" anywhere in GD, this means that some extension is badly broken and can be corrupting the memory of the app. It's almost always because the declaration of the extension is wrong somewhere (here, the usage of an undeclared function it seems).

Let me know if it's better!

_("Choose an object video")
to _("Choose a video object")
@Bouh
Copy link
Collaborator Author

Bouh commented Feb 20, 2019

Pending further instructions on the code, I work on an example with controls.
Video player with subtitles handled in events with Events Functions

@4ian
Copy link
Owner

4ian commented Feb 20, 2019 via email

this._pixiObject = new PIXI.Sprite(this._textureVideo);

this._pixiObject._texture.baseTexture.source.pause();
}
Copy link
Owner

Choose a reason for hiding this comment

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

If this._pixiObject is already defined, this means that the renderer is being "recycled" (when objects are destroyed, they are removed from the scene, put in a cache but not deleted, and then reused to avoid creating/deleting a lot of objects). So the renderer constructor might be called with an existing video - in which case you must reset it to initial value.

else {
  this._pixiObject._texture.baseTexture.source.currentTime = 0;
  this._pixiObject._texture.baseTexture.source.pause();
}

this._pixiObject.rotation = gdjs.toRad(this._object.angle);
};

gdjs.VideoRuntimeObjectPixiRenderer.prototype.updateOpacity = function(number) {
Copy link
Owner

Choose a reason for hiding this comment

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

updateXXX means "read from this._object and update XXX"
setXXX means "I give you some arguments, and you update XXX with it".

Here you're giving an argument, but the method is called updateOpacity.
So either you rename it to setOpacity, or read opacity from this._object for consistency :)

To stay consistent with the other methods, maybe keep "updateOpacity" but read opacity from object.

Copy link
Owner

Choose a reason for hiding this comment

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

You'll also have to divide by 255 then:
this._pixiObject.alpha = this._object._opacity / 255

As stated in another comment, it's usually highly discouraged to access to a property with an underscore. BUT renderers are a special case and can "read inside" their object.

Bouh and others added 11 commits February 24, 2019 01:17
…rash

* Show instance variables in InstancePropertiesEditor
* Display parent variables (not editable) and overriden variables in italic.
* Also Fix (existing) crashes when deleting variables.
  This was wrongly written:
    * no recursive search (`contains` 2nd argument not passed)
    * comparing a VariableAndName with a gdVariable made no sense.
And trying callback
@Bouh
Copy link
Collaborator Author

Bouh commented Feb 24, 2019

Ooops git is not really my friend.

@4ian
Copy link
Owner

4ian commented Feb 27, 2019

I think we should merge master into this, because otherwise the files changed tab is polluted with changes not related to the extension.

And trying callback
@4ian
Copy link
Owner

4ian commented Mar 1, 2019

@Bouh You somehow rebased or got the commit from master and add them to your branch. Have you rebased them instead of merging them?

Anyway, I've fixed your branch. For your interest, here it's the procedure

# Added your repository as a remote called "Bouh"
git remote add Bouh git@github.com:Bouh/GDevelop.git
git fetch --all #Fetch all your branches, notably I'll get one called Bouh/video_extention
git co Bouh/video_extention # I go to your branch
git rebase -i HEAD~15 # I use this to edit the last 15 commits (be careful it's the dangerous part)
# Inside the editor that opened, I remove all the lines with commits that were not related to the video extension. I save and close.
git co -b fixed-Bouh-video-extension # I save my changes in a new branch
git push Bouh fixed-Bouh-video-extension:video_extention -f # I push my branch to your branch, in your repository. I use -f to force git to take my changes (because I rewrote the history)

What you need to do now on your side is to pull this branch again. You might need to delete your local branch, then git pull it again. (because your branch is now different than this one on your repository that I fixed).
Use something like GitHub desktop: https://desktop.github.com/ that should allow you to easily remove your local branch and then get this cleaned one again.

Apart from that, there is:
[ ] The callback handling
[ ] The support for video to add to the image manager.
[ ] Anything else?

@4ian
Copy link
Owner

4ian commented Mar 1, 2019

Nah you've still imported all the outdated commits :/
Can you explain me what have you done to do this? Trying to understand where the error is.

@4ian
Copy link
Owner

4ian commented Mar 1, 2019

I can clean your branch again, but you'll have not to merge anything like you did in it. Instead, delete your local branch and get it back from your repository

@Bouh
Copy link
Collaborator Author

Bouh commented Mar 1, 2019

Sorry, I can delete my local branch video_extention now ? and I pull from Bouh/video_extention
(I'm connected on Discord if you want for share screen, to be sure ^^)

@Bouh
Copy link
Collaborator Author

Bouh commented Mar 1, 2019

Yes i've made a rebase some days ago.
Clean is done in this new branch i've revert the rebase with git revert [SHA], all SHA are in description of last commit in this branch 🤞

@Bouh
Copy link
Collaborator Author

Bouh commented Mar 1, 2019

Or better ! I've copy my branch and reset just before the rebase and i've paste my own code of extention in one commit. See here

@4ian
Copy link
Owner

4ian commented Mar 1, 2019

Clean is done in this new branch i've revert the rebase with git revert [SHA], all SHA are in description of last commit in this branch 🤞

This one still seems broken, because it contains commits like this one:
image

You can see that this commit is "wrong" because GitHub says that I authored and that you commited. This means that you rebased your branch and changed the history (which is fine usually, but not here because there was a merge of master before - keep reading). If you look at the hash on the right (471d7c2), it is different than the hash on the "official" master branch:
image

Hash should stay the same, otherwise git consider them as different commits.
This happen because you may rebased after a merge of master.

Note that rebasing is fine, as long as you're only doing rebases :) As soon as you merge master into your branch, your branch is not clean anymore and can't rebased.
Rebase is basically "rewriting" the history: it's taking the commits on your branch and applying them on the top of another branch (i.e: master in our case). But a "merge" is different, it's taking your branch and master and create a commit where it "imports" the changes from master into your branch.

If you do a merge THEN a rebase, you end up rebasing your commits AND the commits of master on the top of master.. creating issues :)

Next time you need to get updates from master, I'll say "Please merge master in your branch or rebase your branch on top of master". (The advantage of rebase is that you keep a clean history, without merge commit like this:
image)

Or better ! I've copy my branch and reset just before the rebase and i've paste my own code of extention in one commit. See here

Yeah that sounds like a good idea :) So now you can open a new PR with this branch.
As this branch already contains a merge from master into the branch:
image

You need to merge master in your branch to get the latest updates :)

Final note, don't worry about all these "merge commits". When I'm merging PR, I'll "squash" all your commits (including the merges) into a single one, so that all the video extension is in a single, clean commit.

@Bouh
Copy link
Collaborator Author

Bouh commented Mar 1, 2019

See here i've merged if it's ok i open the new PR with it.

@4ian
Copy link
Owner

4ian commented Mar 1, 2019

Yup this is fine 👍 When opening the PR you can see that the commits that are listed are only yours :)

@Bouh Bouh mentioned this pull request Mar 1, 2019
8 tasks
@4ian
Copy link
Owner

4ian commented Mar 2, 2019

Closing this PR as there is a new one open :)

@4ian 4ian closed this Mar 2, 2019
@Bouh Bouh mentioned this pull request Mar 2, 2019
14 tasks
@Bouh Bouh deleted the video_extention branch May 28, 2019 16:19
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.

None yet

6 participants