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

Feature/make radial menu great again #32653

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

Conversation

Fildrance
Copy link
Contributor

@Fildrance Fildrance commented Oct 5, 2024

About the PR

Radial menu was great addition to UI but it requires precision as it is now. To bring up better UX it needs to be usable without too much precision so it can be used effectively in panic situations. This problem was already solved by wheel menu in most modern games - use sectors of circle to pick options and not icons themselves.

Why / Balance

Robust wheel UI!

Technical details

RadialMenu now uses 'special' button as context button in center - RadialMenuContextualCentralTextureButton. It considers space inside inner circle of parent RadialContainer half radius, and ALL outer space outside twice of its radius - as itself.
RadialContainer now puts angle sector data in each child RadialMenuTextureButton.
RadialMenuTextureButton now can draw bagle segments!

Media

new-radial-menu.webm

Requirements

Breaking changes

removed RadialMenuButton - was not used.
now RadialMenuTextureButton requires to be inside RadialContainer to work properly

Changelog

🆑 Fildrance, SaphireLattice, eoineoineoin

  • tweak: now all radial menu have better UX

@github-actions github-actions bot added the Changes: UI Can be reviewed or fixed by people who are knowledgeable with UI design label Oct 5, 2024
@beck-thompson
Copy link
Contributor

add a video to show everyone how cool this is


var dist = Math.Sqrt(Math.Pow(point.X - cX, 2) + Math.Pow(point.Y - cY, 2));
var isNotInRadius = dist > Container.Radius * 2 || dist < Container.Radius / 2;
return isNotInRadius;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this DOES NOT respect not full circle menu! I feel like it will be real pain to make it work with with it. Maybe use default behaviour in this scenario and fallback to texturebutton mouse detection?

@Fildrance Fildrance marked this pull request as ready for review October 7, 2024 17:35
@chromiumboy chromiumboy self-assigned this Oct 8, 2024

// Add padding from the center at higher child counts so they don't overlap.
Radius = baseRadius + (childCount * radiusIncrement);
Radius = baseRadius + (elementCount * radiusIncrement);
Copy link
Contributor

Choose a reason for hiding this comment

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

This adjustment changes the overall area of the container, so there's a large disparity in size between menu layers that contain only a few elements and those that contain many. Consider having a set size for the sectors and only shifting the positions of the icons

On the topic of size, the amount of screen real estate that the sectors take up is quite large, consider reducing the outer radius as well, if it doesn't cause issues with the above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'll remove separators from radius increment - that makes sense, it was my failure to properly understand why it is there.
About size of sectors and 'free-space' - the point of this UX changes was to make it require less precision - so you can click it faster and not really aim, but make choice of direction. Size is just for that - most of games have this 'wheel' ui blur and grow in opacity from center to outer corners - i could not make it work here, sadly (tbf was not trying) but REAL size of radial menus were quite the same overall (if we remove blur and make controls constant color), i belive!
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If there is some proposal about how to change numbers exactly - you are welcome to ask and i will try it - currently its radius * 2 for outer, radius /2 for inner

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed back to only use child count thanks to eoin. Kinda missed it initially. Yay!.
Multipliers were moved to properties - so can be easily changed.

Content.Client/UserInterface/Controls/RadialContainer.cs Outdated Show resolved Hide resolved
Content.Client/UserInterface/Controls/RadialMenu.cs Outdated Show resolved Hide resolved
Content.Client/UserInterface/Controls/RadialMenu.cs Outdated Show resolved Hide resolved
@ps3moira
Copy link
Contributor

ps3moira commented Oct 8, 2024

is there a way to make it smaller? This looks way too big

@gusxyz
Copy link
Contributor

gusxyz commented Oct 8, 2024

is there a way to make it smaller? This looks way too big

Agreed theres a lot of deadspace.

Content.Client/UserInterface/Controls/RadialContainer.cs Outdated Show resolved Hide resolved
Content.Client/UserInterface/Controls/RadialContainer.cs Outdated Show resolved Hide resolved
Content.Client/UserInterface/Controls/RadialContainer.cs Outdated Show resolved Hide resolved
Content.Client/UserInterface/Controls/RadialMenu.cs Outdated Show resolved Hide resolved
Content.Client/UserInterface/Controls/RadialMenu.cs Outdated Show resolved Hide resolved
Content.Client/UserInterface/Controls/RadialMenu.cs Outdated Show resolved Hide resolved
Content.Client/UserInterface/Controls/RadialMenu.cs Outdated Show resolved Hide resolved
Content.Client/UserInterface/Controls/RadialMenu.cs Outdated Show resolved Hide resolved
Content.Client/UserInterface/Controls/RadialMenu.cs Outdated Show resolved Hide resolved
@github-actions github-actions bot added the Status: Needs Review This PR requires new reviews before it can be merged. label Oct 12, 2024
Copy link
Contributor

@metalgearsloth metalgearsloth left a comment

Choose a reason for hiding this comment

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

Isn't it better to just use a shader to avoid the segment overlap? Will also be much cheaper to draw.

@Fildrance
Copy link
Contributor Author

Fildrance commented Oct 13, 2024

Isn't it better to just use a shader to avoid the segment overlap? Will also be much cheaper to draw.

Can this be idea for future improvement? In separate PR.
I have zero idea how to work with shaders and i even was thinking that shaders are not something that CAN work for such scenario. Resources that drawring requires for now looks fine (no alloc on each drawing interation), overlapping of segments that Errant was talking about is not drawing problem and it should not exist currently. 'Overlapping' visual that you can see on screenshot is actually distinct separator line that is coded to be rendered separately (as part of each menu segment)

@chromiumboy
Copy link
Contributor

Isn't it better to just use a shader to avoid the segment overlap? Will also be much cheaper to draw.

Most of our contributers won't have experience with writing shaders, so I don't we can really request that they do so

@lzk228
Copy link
Contributor

lzk228 commented Oct 14, 2024

can you also move text from on hover to make him permanent under or below icons?

@Fildrance
Copy link
Contributor Author

can you also move text from on hover to make him permanent under or below icons?

I belive that part of hover behaviour is from parent component (TextureButton) so that would need... some copy-paste from there. Also not all tooltips would look nice, i feel >_> which is out of current PR scope. I'd better have separate issue for that thing to be fixed and reviewed without all other garbage i coded here

@Fildrance
Copy link
Contributor Author

@eoineoineoin can u please resolve stuff i that was fixed and where answer statisfied you?

@eoineoineoin
Copy link
Contributor

@eoineoineoin can u please resolve stuff i that was fixed and where answer statisfied you?

I don't have any ability to resolve those threads, I'm afraid.

Did you see these comments?
#32653 (comment)
#32653 (comment)

@Fildrance
Copy link
Contributor Author

@eoineoineoin can u please resolve stuff i that was fixed and where answer statisfied you?

I don't have any ability to resolve those threads, I'm afraid.

Did you see these comments? #32653 (comment) #32653 (comment)

Thank you! I will look at that!

@Fildrance
Copy link
Contributor Author

There appears to be a narrow "gap" everywhere the segments join, where it is possible to click through the interface and act on the entity underneath

gap.mp4

Ok i found out why it was happening - float was messing up numbers. I floored position on menu item controls - it snapped to physical grid and al of the math works perfecly now - no holes, not in graphycs, not in HasPoint checks!

@LGRuthes
Copy link
Contributor

Any chance you can have the option to chance opacity on it? That looks too obnoxious for me to use.

@Fildrance
Copy link
Contributor Author

Any chance you can have the option to chance opacity on it? That looks too obnoxious for me to use.

Color (and as result opacity) is a public property of control, so it can be changed. What exact values to use by default - dunno ;(

tb.AngleSectorTo = sepAngle * (childIndex + 1);
tb.AngleOffset = angleOffset;
tb.InnerRadius = Radius / 2;
tb.OuterRadius = Radius * 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

If the inner and outer radii are going to be set by RadialContainer, the multipliers need to be fields, no magic numbers

I'd suggest 1.5f for the the outer radius, there's too much empty space inside the sector otherwise

Copy link
Contributor Author

Choose a reason for hiding this comment

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

extracting property for multipliers - thats actually good idea : >
about 1.5f - ok, but bear in mind WHY we are adding those changes - we want control to be not requiring precision, so you can use it very-very fast. Making it too small make it not UX but just visual change. But having it variable - we can change it in any moment - so its fine.

var type = filled
? DrawPrimitiveTopology.TriangleStrip
: DrawPrimitiveTopology.LineStrip;
drawingHandleScreen.DrawPrimitives(type, _sectorPointsForDrawing, color);
Copy link
Contributor

@chromiumboy chromiumboy Oct 23, 2024

Choose a reason for hiding this comment

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

Suggested change
drawingHandleScreen.DrawPrimitives(type, _sectorPointsForDrawing, color);
drawingHandleScreen.DrawPrimitives(type, _sectorPointsForDrawing, Color.ToSrgb(color));

Drawn primitives require that supplied colors be specified in srgb format

Copy link
Contributor Author

Choose a reason for hiding this comment

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

having to call it on each draw is a bit wierd. But ok. fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's too expensive here, move it else where,or use a lookup table to convert between rgb and srgb colors

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its not like 'too expensive', but we shove everything that sure we can be cached to the cache for perf. Not nice thing is that it will require some wierd getter-setter dancing. I don't mind it, gonna do it...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Content.Client/RCD/RCDMenu.xaml Show resolved Hide resolved
Content.Client/UserInterface/Controls/RadialMenu.cs Outdated Show resolved Hide resolved
Content.Client/UserInterface/Controls/RadialMenu.cs Outdated Show resolved Hide resolved
Content.Client/UserInterface/Controls/RadialMenu.cs Outdated Show resolved Hide resolved
Content.Client/UserInterface/Controls/RadialMenu.cs Outdated Show resolved Hide resolved
Content.Client/UserInterface/Controls/RadialContainer.cs Outdated Show resolved Hide resolved
Content.Client/UserInterface/Controls/RadialContainer.cs Outdated Show resolved Hide resolved
Content.Client/UserInterface/Controls/RadialContainer.cs Outdated Show resolved Hide resolved
pa.pecherskij added 2 commits October 23, 2024 10:51
… menu buttons with sectors, RadialContainer Radius renamed and now actually changed control radius.
@chromiumboy chromiumboy added Status: Awaiting Changes This PR needs its reviews addressed or changes to be made in order to be merged. and removed Status: Needs Review This PR requires new reviews before it can be merged. labels Oct 24, 2024
/// <summary>
/// Color of background in non-hovered state. Accepts RGB color, works with sRGB for DrawPrimitive internally.
/// </summary>
public Color BackgroundColor
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, this is a bit sussy. It's probably better if we just call this field 'BackgroundColorSrgb' (same for those below) and expect people to supply a srgb formatted value rather than this conversion back and forth. Sorry 'bout misleading you on that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is not 'srgb c-tor', so its just plain harder to supply correct thing - i see no reason to enfoce developer to do more action that is actually required. Stylesheet code includes only RGB and not sRGB colors as far as i can see at first glance, so if it will be applied - it should break notion just for this control, and also contain more code then it is crucial to declare stylesheet info (transform to srgb). Getter/Setter logic IS bad when you are going to apply it every second /tick or so, but thats clearly not he case, right?

@Fildrance
Copy link
Contributor Author

enabled any functional keys pressed when pushing radial menu buttons as AI requires alt to be pressed when opening door menu - this will make it feel more fluent as now u dont need release alt when clicking menu, and then press it again to call menu on next door.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changes: UI Can be reviewed or fixed by people who are knowledgeable with UI design Status: Awaiting Changes This PR needs its reviews addressed or changes to be made in order to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.