better performance for Menus and fix clicks on items #2299
Conversation
Compile Times benchmarkNote, that these numbers may fluctuate on the CI servers, so take them with a grain of salt. All benchmark results are based on the mean time and negative percent mean faster than the base branch. Note, that GLMakie + WGLMakie run on an emulated GPU, so the runtime benchmark is much slower. Results are from running: using_time = @ctime using Backend
# Compile time
create_time = @ctime fig = scatter(1:4; color=1:4, colormap=:turbo, markersize=20, visible=true)
display_time = @ctime Makie.colorbuffer(display(fig))
# Runtime
create_time = @benchmark fig = scatter(1:4; color=1:4, colormap=:turbo, markersize=20, visible=true)
display_time = @benchmark Makie.colorbuffer(display(fig))
|
|
Some more interaction issues/differences:
|
|
Thanks for the thorough review :) I should have fixed all problems now - and found a new appreciation for mousestatemachine :D mousestate1 = mousestatemachine(scene, plots...)
mousestate2 = mousestatemachine(scene, other_plots...)
on(events.mouse) do mouse
if ishover(mousestate1, mouse)
....
end
if ishover(mousestate2, mouse)
....
end
endWell, that's for another PR and I also still need to take a closer look at mousestatemachine. Maybe it's not as bad to have one observable and multiple functions registered...But then one needs to fight for the event via consume, which seems easier if the whole state of something complex as a menu is handled in one function 🤷 |
|
If mouseevents ignores clicks it's probably because they are registered on multiple objects or scenes at once I think. Unless the mouse down and mouse up events are already not received. But if they are, then some other condition must not be met, which could be a bug in the implementation but also in our event system. |
Missing reference imagesFound 1 new images without existing references. |
|
Back when I reworked the event system (i.e. introduced priority) I also noticed some differences between how mouse state machine works and how my OS reacts. I reworked it back then but didn't push it because I didn't want to add more complexity to the pr. I noted some of the oddities in the tests but I never got back to making a pr for it... Lines 361 to 435 in 087dfbd |
|
ah good that you collected some of those, it's much better to test them in this way, although I only found some of the edge cases by trying to break the code and acting weird with the gui. Some of these cases are not well defined but I think if the big OSes differ in behavior we should use that as reference. Like with the mentioned case where left and right click happen in a nested fashion |
|
I say we merge this and leave further refactors & tests of the event system / mousestate machine for another day? |
|
There are still some issues:
|
|
Are you sure you have all the new changes? That sounds exactly like all the issues I fixed... |
|
Oh nvm, I didn't pull correctly. Looks good to me 😆 |
* better perf + fix clicks on items * fix menu behaviour * clean up and comment * clean up and reference test * fix tests * don't make background black
addmouseevents!seems to ignore clicks and I couldn't really get to the bottom of the problem :(I started using
scene.eventsdirectly to debug things, and since it fixed the issue, should be faster, uses less listeners and does all the work in one function, i sticked with that solution ;)I'm not 100% sure though, if it's correct in regards to Consume behaviour, but it seems to work pretty well.
We should also think about adding a z-index value, since in some cases the z-value seems too low.
Maybe, we should also think about having
overdraw=truefor scenes, so that we can better create pop-up scenes without worrying about any z-index.This also defaults to the first item in the option list instead of
nothing, which at least to me seems more practical. Willing to remove that change, though ;)