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

Circle menu fixes #2904

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

RedSkyByte
Copy link

I started implementing what we arrived at in discussion of #2895 :

  • 1. Backdrop field that allows you to be less precise with your clicks when choosing menu buttons
  • 2. Clicking outside of menu will close it.
  • 3. selection on keyup/mouseup
  • 4. tap again key to close
  • 5. permit movement with menu opened
  • 6. refactor lua code

Very humble changes, but already major change in usability. I'm looking for some advise and input.

I can't progress on 3. and 4. before I can query current binds (so i know what keys to be on the lookout).
With no. 3 there is also problem with building - currently mouse press that spawns menu doesn't emit mouseup when released.
I can't do anything on 5. without basing it on #2900

I can jump into refactoring, but I'd like to know if armory menu will stay as is and if what I changed so far makes sense.
Lua code that needs to be abstracted namespaced better which might be needed if complexity of UI will rise. Other then that I'm also thinking data binding(MVC) for displaying up-to-date information live and less hacky menu building.

@DolceTriade
Copy link
Member

am testing this. thanks for doing this!

@DolceTriade
Copy link
Member

I didn't review the code, but the changes feel good. I think that its quicker to exit the menu and select things.

I wish there was a way to display the circle menu as a a "pie slice" so we can highlight the whole slice, but the work here is a clear improvement.

Copy link
Contributor

@slipher slipher left a comment

Choose a reason for hiding this comment

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

It's pretty nice. I noticed a couple of minor aesthetic issues (non-merge-blocking):

  • In the Armory menu, currently owned items do not highlight when the cursor is in their sectors. But a currently owned item is selectable (clicking it sells the item.)
  • It looks kind of bad when the crosshairs of some weapons are drawn in the space between the cancel button and the options.

pkg/unvanquished_src.dpkdir/ui/lua/util.lua Outdated Show resolved Hide resolved
Copy link
Contributor

@slipher slipher left a comment

Choose a reason for hiding this comment

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

Looks almost ready.

Note that if you are ready for the code to be reviewed again, it is good to make a comment to ensure that this will be noticed.

pkg/unvanquished_src.dpkdir/ui/lua/circle_menu.lua Outdated Show resolved Hide resolved
pkg/unvanquished_src.dpkdir/ui/lua/circle_menu.lua Outdated Show resolved Hide resolved
@RedSkyByte
Copy link
Author

Thank you for your time. I got preoccupied this week, before I managed to tidy up my refactoring.

Note that if you are ready for the code to be reviewed again, it is good to make a comment to ensure that this will be noticed.

Yes, Sir, I'll do that. I was planning on signalling somehow readiness for review after I removed ugly regressions my refactoring introduced and now I know how.

Refactoring(and cleanup) of lua is the easy part. The harder part is advanced key handling, which requires querying client key binds. BTW, build menus seem to ignore their first (inducing) mouse click which they shouldn't do if a single click confirm-on-release selection is to be a thing. So another thing potentially to be done.

@Grise3
Copy link
Contributor

Grise3 commented Feb 8, 2024

@RedSkyByte If you need help with something I know how to do, don't hesitate do ask me

@slipher
Copy link
Contributor

slipher commented Feb 10, 2024

Refactoring(and cleanup) of lua is the easy part. The harder part is advanced key handling, which requires querying client key binds. BTW, build menus seem to ignore their first (inducing) mouse click which they shouldn't do if a single click confirm-on-release selection is to be a thing. So another thing potentially to be done.

For a single-press mode the commands would probably need to be changed to "plus" commands which allow responding to key up. See for example +scores which shows the player list when held. I don't think querying binds is a good approach for anything except the bind configuration menu.

A shallow Object Oriented abstraction of circle menus.
Concrete implementation derive from CircleMenu prototype.
They must handle data acquisition, displaying info and option selection.
In turn, objects inherit from prototype all mechanisms for building menu
and all major event handlers.

All these mechanisms centralised in CircleMenu object's methods, but can
be easily overwritten to handle exceptional behaviour.
@RedSkyByte
Copy link
Author

I should have force pushed rebase without last commit. My changes are far less readable now. I'm sorry.
Major regressions removed from refactor. Code cleaned up.

For a single-press mode the commands would probably need to be changed to "plus" commands which allow responding to key up. See for example +scores which shows the player list when held. I don't think querying binds is a good approach for anything except the bind configuration menu.

There is no other clear way to implement select-on-release with exception of building of course. As reminder: I don't want to issue commands here, I want to consume inputs and interpret what they mean. Combined with support for movement with menu open, it's not a trivial problem.

event:StopPropagation()
self.menuElement.child_nodes[index+1].first_child:Click()
end
end

function CircleMenu:Quit(event)
Copy link
Contributor

Choose a reason for hiding this comment

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

What's with this super complicated quit thing? It worked fine before without that.

Copy link
Author

Choose a reason for hiding this comment

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

It worked fine before without that.

Not exactly, there was a lot of corner cases of undesired behavior around clicking and click-releasing on buttons and backdrop, closing menu or leaving it open when it shouldn't.

Managing events from click events is a bit tricky, especially since onclick, onmouseup and onmousedown are separate. I also found that one click event on body element tends to leak out and fire after menu document is closed, no matter what I do.
And like I mentioned that in comments, trying to hide document already hidden (and deallocated, I presume) does, on occasion, crush the program.

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this is due to us using this Events.push_event hack instead of calling the methods directly. I think Events.push_event() causes the action to occur on the next frame instead of immediately.

So, in most places where you do Events.pushevent("hide foo"), you could probably do self.document:Blur()

This should cause the hide action to take effect immediately.

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

4 participants