ReaTeam / ReaScripts-Templates

Templates, models, boilerplates, examples and snippets for REAPER ReaScript.
70 stars 23 forks source link

Small fixes and improvements to GUI framework #2

Closed firthm01 closed 6 years ago

firthm01 commented 6 years ago

Various small fixes/improvements to problems I encountered using Lokasenna's GUI framework;

X-Raym commented 6 years ago

@firthm01 Thanks for your contribution ! Looks like nice improvements.

@jalovatt Can you take a look ? 😃

jalovatt commented 6 years ago

Thanks for doing this, @firthm01. I have zero time for coding these days so it's nice to have someone else lend a hand.

I only have time for a quick look, but:

Don't render text - Oops. Yes.

Non-existent function - I'm an idiot. Thanks.

GUI.z_max becoming stale - I limited z_max to the initialization loop because I figured it might end up being a lot of calls to math.max() every update cycle in a GUI with potentially dozens of elements. No idea how much time it actually saves, though. Thoughts?

More robust update cycles - That definitely looks like a smarter way to do it.

Dragging - The existing code already lets you drag outside an element without breaking the drag. This does seem like a more efficient way to do it though, using GUI.mouse_down_element in place of all the GUI.IsInside calls. Clickthrough should (I think) already be prevented - z-layers are updated from the top down, and Update quits once an element successfully takes input. No?

x/y_delta to :ondrag - I'm not sure what this offers that can't be done by checking mouse.lx, but I'm okay with it. Is it just in case someone wants to modify :ondrag and have it available?

delta to :onwheel - See ^.

Reset z_max: See my response about z_max above. I have no objection, just not sure if the extra math would be a CPU hog.

Cheers.

X-Raym commented 6 years ago

@jalovatt let us now if it can be merged :P

jalovatt commented 6 years ago

I listed a couple of concerns/questions; I'd rather wait until those are addressed.

firthm01 commented 6 years ago

No problem, I'll try explain my reasoning for some of the changes. Please feel free to reject particular modifications as you like, I won't be offended :) I pretty much modified for my own use case but thought some of the changes may be useful to you/others. Thank you for your work on this by the way! It's a great framework and you've saved me a heap of dev time.

GUI.z_max becoming stale - I limited z_max to the initialization loop because I figured it might end up being a lot of calls to math.max() every update cycle in a GUI with potentially dozens of elements. No idea how much time it actually saves, though. Thoughts?

There were a few reasons I needed to ensure this was getting recalculated. My initial reason was that in my particular use case, I was able to go in to a mode that had relatively deep z-layers (around 30). On exiting this mode, I was only using 2 or 3 layers but the for loops in the update function were still needlessly iterating 30 or so times. Also in my script I am able to effectively 'send to back' on elements, or create new elements on new layers. These actions can populate new Z-layers deeper than z_max which then weren't included in the update loops. It may be worth profiling math.max - I expect that a simple if z > GUI.z_max may be better - that should be very lightweight.

Dragging - The existing code already lets you drag outside an element without breaking the drag.

I think there may be confusion here. This is true if the element remains in the same place throughout the drag. However, if drag is used to 'grab and move' an element by changing its coordinates in the ondrag handler (as in my use case), the element boundaries can move so far as to exclude the location of the original mouse-down click which then breaks the drag. This is because mouse.ox and mouse.oy are updated within the if not GUI.mouse.last_down condition, and these were the coordinates used to determine if we were dragging a particular element (using GUI.IsInside(elm, GUI.mouse.ox, GUI.mouse.oy)). This returns false if the boundaries of elm no longer include mouse.ox and mouse.oy and so ondrag is no longer called and the drag behaviour is broken.

Clickthrough should (I think) already be prevented - z-layers are updated from the top down, and Update quits once an element successfully takes input. No?

I think it was at one stage, but the line that would have done this (if GUI.elm_updated then break end) has been commented out for some reason. I expect this is because when one element 'claims' a mouse action, the element that should call lostfocus may not do so because the loop is broken and the element is never passed to GUI.Update().

x/y_delta to :ondrag - I'm not sure what this offers that can't be done by checking mouse.lx, but I'm okay with it. Is it just in case someone wants to modify :ondrag and have it available?

delta to :onwheel - See ^.

That's the only reason for it really. My handlers needed to know the amount of change and since the update functions also needed to know if there was any change, it seemed to make sense to just calculate it once and pass it though (although admittedly doing the extra subtraction operation isn't going to make any noticeable performance difference).

jalovatt commented 6 years ago

Fair enough, and yes - I remember commenting the GUI.Update out... I think it was, as you say, to do with :lostfocus().