Hover #898
Hover #898
Conversation
| @@ -73,6 +76,27 @@ function refresh(root) { | |||
| } | |||
| } | |||
|
|
|||
| function update_cursor() { | |||
There was a problem hiding this comment.
This could feasibly live in the controller too, happy to go with whatever was more in line with the intention.
There was a problem hiding this comment.
It does feel like it needs a better home, but I'm not sure there is one that isn't just needless indirection.
However, is there a reason to check for each global_button_action.action explicitly? Wouldn't this be equivalent?
canvas.style.cursor = tree_state.mouse_hold ? "move" : global_button_action.action ? "pointer" : "default"I can't see anything in live_area_config.js that hasn't been listed.
If we do want a live area with a different cursor, which seems unlikely, then I think adding that to a tree_state.cursor inside the register_button_event() functions would be the way to go.
There was a problem hiding this comment.
Haha, yeah I think to start with I'd tried 'zoom-in' cursor for 'tap2zoom' etc. but decided it looked silly so the switch statement is spare now. Have simplified and also added in a little check to make sure we don't steamroll over the mouse interactor's 'zoom-in'/'zoom-out' cursor setting.
| if (tree_state.button_x != last_button_x || tree_state.button_y != last_button_y) { | ||
| last_button_x = tree_state.button_x; | ||
| last_button_y = tree_state.button_y; | ||
| return true; |
There was a problem hiding this comment.
In principle I expected that we'd be able to call get_shapes in here rather than returning true, since that's (as far as I understand) the thing that is actually updating global button action and then the next line would return true if necessary, which would be obviously better, but I couldn't get that to work at first glance, it just got stuck not rendering at all.
There was a problem hiding this comment.
I'd assume there's something seeing the second call to get_shapes() with an identical tree and deciding rendering doesn't need to happen, but what that is I dunno.
Anyway, how about not bothering with last_button_x, and moving the get_shapes() call outside of if (need_refresh())?
OZtree/OZprivate/rawJS/OZTreeModule/src/render/renderer.js
Lines 59 to 60 in 5eff752
This is more wasteful admittedly, as we're then running get_shapes() on every refresh rather than every refresh with a moving mouse cursor. But it's just get_shapes() not refresh_by_redraw(), and if there's no activity the whole refresh loop will fall idle & stop anyway.
We could make it 2 stage maybe, a need_shapes() & need_refresh(), but a quick profile shows that get_shapes() suggests that get_shapes isn't that expensive in comparison to the actual render (which is mostly slowed down by text measuring).
There was a problem hiding this comment.
Hmm. Trying that, I'm finding I get a fiar bit of garbage collection cruft and stuttering when running at 120Hz. I'm not being very scientific about it though.
I've pushed a commit to pull the get_shapes outside of the need_refresh only when the cursor has moved which seems to work okay. See what you think of that.
|
Thanks very much for the comments Jamie, appreciate it. |
|
Thanks for looking at this @lentinj . I'm not familiar with this bit of the code, so am probably not best placed to test it out. Once you and Jared are happy with it, I'm fine to merge it, however. As with the other PRs, we can sync up the |
|
I just tried this, and it seems like a nice improvement. |
|
Happy for you to merge in general @lentinj , but I mighty as well do this one since I'm here. |
Kinda relates to the remaining task on #155 i.e. fixes slow hovering but also adds a cursor:pointer during hover states.
Before:
https://github.com/user-attachments/assets/4dd9ca85-9338-455d-83a6-96de79a3bf70
After:
https://github.com/user-attachments/assets/b1974ae3-5d59-445d-a54c-081d93f40c14
Fixes #155