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

Implement a way to update only changed effects. #5

Closed
QOAL opened this issue Oct 26, 2013 · 16 comments
Closed

Implement a way to update only changed effects. #5

QOAL opened this issue Oct 26, 2013 · 16 comments
Labels

Comments

@QOAL
Copy link

QOAL commented Oct 26, 2013

Currently to update an effect you have to load the entire preset again, which causes a noticeable pause.

This produces a poor experience when editing a preset using AVS style editor, where you (historically) expect the preset to update on the fly.

There are several ways a preset can change:
An effect being modified, an effect being added or removed and an effect being moved around the tree. (I think that covers all cases)

In the case of an effect being modified, being able to mark the effect as needing a rebuild and calling a function that scans the preset looking for marked effects seems like a good way to handle it.
I'm unsure about the other cases.

Thoughts on this?

@azeem
Copy link
Owner

azeem commented Oct 26, 2013

+1 for this. I guess it gets particularly slow because of the webgl shader recompilation + the avs expression recompilation.

I was thinking something along these lines: All preset component definitions can have an 'id'. Webvs.Main will have a partialLoad function which accepts a single component definition. This function would then instantiate and replace only that specific component in the tree. The 'id' could be string so that it can be used as a readable name in the editor tree, instead of simply showing the effect name. what do you think?

@azeem
Copy link
Owner

azeem commented Oct 26, 2013

On second thought. we would need actual tree operations on main wouldnt we?
something like

  • addComponent(parentId, component)
  • moveComponent(componentId, newParentId)
  • removeComponent(componentId)

@QOAL
Copy link
Author

QOAL commented Oct 26, 2013

Yeah, that sounds like the cleanest approach to me.

With partialLoad renamed updateComponent(componentId) to match the others.

azeem pushed a commit that referenced this issue Oct 30, 2013
@QOAL
Copy link
Author

QOAL commented Oct 31, 2013

Maybe I'm being dense, but how you supposed to get the component id in order to use these functions?

@azeem
Copy link
Owner

azeem commented Oct 31, 2013

yes, this is an issue.

Firstly, ids can be specified in the JSON. If not, an id will be autogenerated when the effects are created.

Secondly, letting the frontend know about the ids and the current webvs internal effect tree structure, could be a bit tricky. i was thinking, if there is some sort of a traversal method like 'traverse(callback)' that gives you this, then something like

main.traverse(function(childid, parentid, options) {
  //do your UI tree building logic, here
} );

will allow us to build the tree in the UI. what do you think?

On a side note, this feature messes up the clone property. Basically, clones work currently by simply creating multiple instances of the same component in the tree. So, now, since we have all these tree operations, the clones should be handled consistently eg. moving an effect with clone property should move all of its clones together and so on. This wasnt a problem previously, since the only way to operate on the tree was to reload everything altogether, so the frontend never got to know about the multiple clones being maintained internally. I have come up with a solution but im afraid it is making things a bit complicated. what do you think of the clone feature? i quite like it, dont have the heart to drop it.

@QOAL
Copy link
Author

QOAL commented Oct 31, 2013

Ahh, this does sound like a problem.

Hmm... well you can already traverse the internal copy of the preset (webvs.preset), but if there were a way to do the same through your traverse function then I can see that working, yes.

(My brain wasn't switched on when I first read that idea and I ended up writing the below)

From my perspective managing the internal copy of the preset (webvs.preset) might make sense, although that could make things a little weird.

Say as the preset is built by webvs, each component adds its ID (if it doesn't have one) to its node in webvs.preset.
You would then be able to easily find the component that is being worked on in the editor, its internal ID and then perform the necessary tree operation.
However tree operations would have to carried out on webvs.preset along with the actual internal version, so things stay in sync.
That would also mean an editor would use webvs.preset rather than its own preset object, otherwise things would get silly having to mirror it.

I'm not sure if that would be the best way, or what would be. Maybe yours would be.
It's something that needs to be done the most bullet proof way first time.

As for clones, it's a nice feature. It doesn't get in the way of any AVS presets or functionality and it can be used to reduce duplicate code, which everyone likes.

Would marking them internally as clone of a component not help with tree operations? Perhaps treating the first one as the "real" component (maybe storing the number of clones of it with it) and then marking the rest.
I would think doing that should help any tree operation.

You need to move something to below this cloned superscope? Okay, well is this target position a clone node? Yeah? Okay well go up the tree (lame thing to do, I know) until you find the "real" one and find out how many clones it has, then offset the target position correctly.

Would doing it like that be viable and less complicated?

@azeem
Copy link
Owner

azeem commented Nov 1, 2013

webvs.preset doesnt do much really. Its just a reference to the preset json that you pass in to loadPreset, it is used to sort of reset things in case the webgl context is lost or reset. Infact i think it doesnt make much sense to have it now, will have to clean it up. I think i understand what you are talking about. Eventually, for the UI, it all comes down to syncing tree state with Webvs.

traverse is infact, a mechanism to keep the UI tree and Webvs tree in sync, albeit a really simple one. Most of the time the UI could just modify its representation (the DOM or tree widget model or whatever) separately (UI should have all the information to do this, whats missing i think is that the addComponent method should give back the id of a newly created component). Since the UI alone is initiating all the tree manipulating operations, Webvs and UI will be in sync.

The place where UI could go out of sync is when we load a preset. After loading a preset, UI could use traverse to instantly rebuild UI so that it is now, as before, in sync with Webvs.

As for the clone, yes this is exactly how i ended up writing the code. It gets a bit complicated when you realize that you can clone Effectlists as well. Moving another component (cloned or otherwise) into or out-of such an effectlist will require us to increase or decrease the number of clones correspondingly and redistribute them among the Effectlist clones. And this has to happen all the way through no matter how many cloned Effectlists there might be along the path.

@grandchild
Copy link
Contributor

I haven't looked at the code in detail, but just from reading this thread, wouldn't it make sense to have the components in the tree only store their multiplicity? By this I mean that you could store any effect's clones just as a number n and then have the rendering loop them n times (with index substitution of course). This way you wouldn't have to maintain duplicated effects anywhere, neither internally nor in the UI.
Do tell me if I should just 'go and read the code' before making any comments, but it seems to me this would be the straightforward way to handle clones...

@azeem
Copy link
Owner

azeem commented Nov 1, 2013

@grandchild yes that could work if the superscope code doesnt rely on the state of the previous frame. But effects are a lot more than just a bunch of code followed by rendering. The effect encapsulates the states of the avs variables as well. The values of variables persist, across frames. (for eg: if you do a=a+1 in a supersope perFrame code, its value increases for each frame). Multiple effect usages actually rely on this to render properly eg: in JelloCube superscope code, each render relies on the curve control points from the previous render, and since each face of the cube has to work independently we need that many clones of the scope instead of loops.

azeem pushed a commit that referenced this issue Nov 4, 2013
@QOAL
Copy link
Author

QOAL commented Nov 6, 2013

I've encountered a couple of typo's while trying to implement the new methods. (patch: http://pastebin.com/raw.php?i=yKUck3ZA - cba to fork just for typos)

Anyway, I've found that webvs just stops rendering with no errors thrown when I update a component.
I'm a bit confused by that.

Found a bug too while removing things in the Jello cube preset, if you remove the top buffer save:

[16:48:10.803] Error: WebGL: framebufferTexture2D: texture: deleted object passed as argument @ http://localhost/webvseditor/qios-webvseditor/webvs.js:2141
[16:48:10.803] Error: WebGL: framebufferRenderbuffer: renderbuffer: deleted object passed as argument @ http://localhost/webvseditor/qios-webvseditor/webvs.js:2142

(I'm also using getPreset() to get the IDs at the moment, rather than traverse, while I walk my editors internal preset as it seems simpler)

@azeem
Copy link
Owner

azeem commented Nov 6, 2013

Thanks. Applied patch + fixed some of the problems. That was a lot of silly mistakes 😁 I have tested updates with the editor. I'm sure there are still more, so i've pushed the changes to 1.1dev branch, once we've ironed out the rough edges, i'll patch the changes back to master.

The editor is looking awesome btw. i like the textbox popout feature. One thing that i noticed, is that even with these changes there is still quite a bit of lag when updating some of the components, particularly the cloned ones. I guess live updating the components (as opposed to replacing with new instance) has to be done. As you've mentioned, It is a bit important because live-editing is sort of the defining and perhaps most useful feature of AVS.

@QOAL
Copy link
Author

QOAL commented Nov 6, 2013

Great, thanks for those fixes.

I found my addThisEffect function was actually trying to use updateComponent rather than addComponent, so silly mistakes all round.

  • Updating the root node (to toggle clear every frame) throws an error in libs.js
  • I found code can't be blank or an error is thrown (See the default SuperScope code, its onBeat is blank)

I still have to get the move function sorted out, I think that is the last thing, other than testing.

Thank you, I thought it was a nice addition.

Yeah, the clones are noticeable slow with the Jello Cube. (But otherwise it's nice and quick :)
Are you able to do something a bit like Grandchild suggested, where you build/compile the initial clone and then copy the result n times - and cid replaced with the correct clone id? Would that work and allow Global/Local vars to be unaffected?
Also we're getting way off topic here, but if an AVS were to be converted to webvs and it used the var cid, but no clones, would there be any issues?

@azeem
Copy link
Owner

azeem commented Nov 7, 2013

Fixed those problems too. I am trying a slightly different approach based on Grandchild's idea, which would greatly simplify and speed up things, but restricts cloning to only some effects viz. ssc, dm, texer etc. Which is okay IMO because cloning really only makes sense for things that have code in them.

@QOAL
Copy link
Author

QOAL commented Nov 7, 2013

That sounds promising, and I agree that only coded effects benefit from clones.

moveComponent lacks a pos argument, could that be implemented please?

Noticed a bug in SuperScope colour rotation. It only uses the first colour once, then loops any additional colours.

@QOAL
Copy link
Author

QOAL commented Nov 10, 2013

Well I think this is fixed now, and the cloning speed up is very quite noticeable.

Unfortunately I've been experiencing some strange behaviour with my editor when the webvs component moving code is enabled, it works fine commented out. So I can't say for certain that there are no bugs.

Any remarks before this is closed?

@azeem
Copy link
Owner

azeem commented Nov 11, 2013

I have tried moving with the editor and it seems to be working fine.

@QOAL QOAL closed this as completed Nov 11, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants