-
Notifications
You must be signed in to change notification settings - Fork 23
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
Pano zoning UI #133
Pano zoning UI #133
Conversation
10b1ecf
to
368b5f7
Compare
553c9d0
to
f456304
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lot of nits and small TypeScript things, sorry to be annoying. Generally, you shouldn't need to cast at all as much as you're doing. We should probably turn off strict mode in the future anyway, but to assert that something is non-null to make TS happy, use !
instead of casting T | null
to T
.
Haven't read everything in detail yet, wanted to do a first pass tonight . I'll try stuff out in-game and give some UI/UX comments tomorrow.
Great work getting this in!
<Button id="ZoningOpen" class="button horizontal-align-left" onactivate="GameInterfaceAPI.ConsoleCommand('mom_zone_edit 1');$.DispatchEvent('ZoneMenu_Show');" onmouseover="UiToolkitAPI.ShowTextTooltip('ZoningOpen', '#Zoning_ShowMenu_Tooltip');" onmouseout="UiToolkitAPI.HideTextTooltip();"> | ||
<Image class="button__icon" src="file://{images}/pencil-outline.svg" textureheight="24" /> | ||
</Button> | ||
<Button id="ZoningClose" class="button horizontal-align-left" onactivate="GameInterfaceAPI.ConsoleCommand('mom_zone_edit 0');$.DispatchEvent('ZoneMenu_Hide');" onmouseover="UiToolkitAPI.ShowTextTooltip('ZoningClose', '#Zoning_HideMenu_Tooltip');" onmouseout="UiToolkitAPI.HideTextTooltip();"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these really need to be console commands? We're likely to remove GameInterfaceAPI.ConsoleCommand
in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, this is kind of tech debt.
- My original intent was to allow a player to start zoning by setting
mom_zone_edit 1
, but if it's Ok to only get into the zoning UI with a button on the tab menu, no sweat. - There are some things handled in c++ by the existing zone editing code that need to be moved over to the new files. These are convars like grid size, and the code that generates the 3D crosshair.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay nbd - what's the long-term plan then? Note that we have $.RegisterConVarChangeListener
now!
//@ts-expect-error property must be optional | ||
delete region.teleDestPos; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Grr
Don't think we should be merging this into |
e2835f6
to
4514331
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
took a first pass at these comments. Still got a few to address
<Button id="ZoningOpen" class="button horizontal-align-left" onactivate="GameInterfaceAPI.ConsoleCommand('mom_zone_edit 1');$.DispatchEvent('ZoneMenu_Show');" onmouseover="UiToolkitAPI.ShowTextTooltip('ZoningOpen', '#Zoning_ShowMenu_Tooltip');" onmouseout="UiToolkitAPI.HideTextTooltip();"> | ||
<Image class="button__icon" src="file://{images}/pencil-outline.svg" textureheight="24" /> | ||
</Button> | ||
<Button id="ZoningClose" class="button horizontal-align-left" onactivate="GameInterfaceAPI.ConsoleCommand('mom_zone_edit 0');$.DispatchEvent('ZoneMenu_Hide');" onmouseover="UiToolkitAPI.ShowTextTooltip('ZoningClose', '#Zoning_HideMenu_Tooltip');" onmouseout="UiToolkitAPI.HideTextTooltip();"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, this is kind of tech debt.
- My original intent was to allow a player to start zoning by setting
mom_zone_edit 1
, but if it's Ok to only get into the zoning UI with a button on the tab menu, no sweat. - There are some things handled in c++ by the existing zone editing code that need to be moved over to the new files. These are convars like grid size, and the code that generates the 3D crosshair.
scripts/pages/zoning/zoning.ts
Outdated
} | ||
|
||
const selectButton = newTracklistPanel.FindChildTraverse('SelectButton') as Panel; | ||
if (selectButton && object) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope, it's always defined. fixed
scripts/pages/zoning/zoning.ts
Outdated
const expandIcon = newTracklistPanel.FindChildTraverse('TracklistExpandIcon') as Panel; | ||
const collapseIcon = newTracklistPanel.FindChildTraverse('TracklistCollapseIcon') as Panel; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep
if (teleDestIndex === 0) { | ||
// no teleport destination for this region | ||
region.teleDestTargetname = ''; | ||
//@ts-expect-error property must be optional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this comment, let's chat about it
523e959
to
48b839b
Compare
48b839b
to
c71f52d
Compare
finally pushed the changes I made while flying over the weekend. This includes the margin and alignment changes we discussed |
c71f52d
to
bcdbd22
Compare
<Button id="ZoningOpen" class="button horizontal-align-left" onactivate="GameInterfaceAPI.ConsoleCommand('mom_zone_edit 1');$.DispatchEvent('ZoneMenu_Show');" onmouseover="UiToolkitAPI.ShowTextTooltip('ZoningOpen', '#Zoning_ShowMenu_Tooltip');" onmouseout="UiToolkitAPI.HideTextTooltip();"> | ||
<Image class="button__icon" src="file://{images}/pencil-outline.svg" textureheight="24" /> | ||
</Button> | ||
<Button id="ZoningClose" class="button horizontal-align-left" onactivate="GameInterfaceAPI.ConsoleCommand('mom_zone_edit 0');$.DispatchEvent('ZoneMenu_Hide');" onmouseover="UiToolkitAPI.ShowTextTooltip('ZoningClose', '#Zoning_HideMenu_Tooltip');" onmouseout="UiToolkitAPI.HideTextTooltip();"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay nbd - what's the long-term plan then? Note that we have $.RegisterConVarChangeListener
now!
</ToggleButton> | ||
</Panel> | ||
</ContextMenuCustomLayout> | ||
</root> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing newline at end?? Thought Prettier handled this, weird
expandIcon.SetHasClass('hide', !shouldExpand); | ||
collapseIcon.SetHasClass('hide', shouldExpand); | ||
const parent = container.GetParent(); | ||
if (parent && parent.HasClass('zoning__tracklist-segment')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parent?.HasClass
Closes momentum-mod/game/issues/2122
This PR implements an in-game zoning tool to replace the previous VGUI implementation. The new UI is based on the zone definition refactor to be released with version 0.10 of Momentum Mod.
Depends on red change before merging.
Checks
feat: Add foo
,chore: Update bar
, etc...fixup
ed into my original commits.POEditor JSON Strings (if needed)