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

hud customizer: Add resizing #138

Merged

Conversation

wertiop121
Copy link
Contributor

@wertiop121 wertiop121 commented Aug 6, 2024

Added resizing to the hud customizer.

Most hud elements aren't resized properly, but the chat is.
There's a new size property that needs to be added to the hud_default.kv3

Checks

  • I have thoroughly tested all of the code I have modified/added/removed to ensure something else did not break
  • I have followed semantic commit messages e.g. feat: Add foo, chore: Update bar, etc...
  • My branch has a clear history of changes that can be easy to follow when being reviewed commit-by-commit
  • My branch is functionally complete; the only changes to be done will be those potentially requested in code review
  • All changes requested in review have been fixuped into my original commits.
  • Fully tokenized all my strings (no hardcoded English strings!!) and supplied bulk JSON strings below

@wertiop121 wertiop121 force-pushed the feat/hud-customizer-v1 branch from 9485bb0 to 5264f76 Compare August 6, 2024 15:23
@tsa96
Copy link
Member

tsa96 commented Aug 7, 2024

Exciting stuff, looking forward to trying this out! I might need a few days before I can review this properly (definitely need to try out ingame).

If you want stuff in the meantime, we're going to need to change layouting of every HUD component to take resize better (a lot of stuff has width: 100% and just fills the entire screen atm). Probably quite a boring but someone's gonna have to do it eventually, and once done we'll be getting close to this being the default HUD behaviour for everyone.

Copy link
Member

@tsa96 tsa96 left a comment

Choose a reason for hiding this comment

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

Really tired atm so might be missing, gonna re-read tomorrow but got a few comments.

I tried out ingame and one issue was the resize knobs moving with the virtual panel, not the snapping HUD panel. This looks very strange and not something we want, even if it makes the code more complicated, sorry. Besides that it seems to function well!

Comment on lines 58 to 65
TOP = 1,
TOP_RIGHT = 2,
RIGHT = 3,
BOTTOM_RIGHT = 4,
BOTTOM = 5,
BOTTOM_LEFT = 6,
LEFT = 7,
TOP_LEFT = 8
Copy link
Member

Choose a reason for hiding this comment

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

Clearer if the non-move variants are prefixed with RESIZE_

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

Comment on lines 43 to 54
function getResizeVector(dir: number) {
return [
[0, -1],
[1, -1],
[1, 0],
[1, 1],
[0, 1],
[-1, 1],
[-1, 0],
[-1, -1]
][dir - 1];
}
Copy link
Member

Choose a reason for hiding this comment

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

Much better if we can pass an enum value here - I would just use a map, something like
resizeVector: ReadonlyMap<Exclude<DragMode, DragMode.MOVE>, [number, number]>

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

@wertiop121 wertiop121 force-pushed the feat/hud-customizer-v1 branch 2 times, most recently from ace2aa4 to 95f6010 Compare August 8, 2024 16:19
@wertiop121
Copy link
Contributor Author

I tried out ingame and one issue was the resize knobs moving with the virtual panel, not the snapping HUD panel. This looks very strange and not something we want, even if it makes the code more complicated, sorry. Besides that it seems to function well!

I made the knobs stick to the hud panel.

Should I squash your commit?

@tsa96
Copy link
Member

tsa96 commented Aug 8, 2024

Fixup is fine

@tsa96
Copy link
Member

tsa96 commented Aug 8, 2024

I'll try out again tomorrow, thanks for fast response!

@wertiop121 wertiop121 force-pushed the feat/hud-customizer-v1 branch 6 times, most recently from 0e8d7d3 to fed00ea Compare August 10, 2024 08:36
@wertiop121 wertiop121 force-pushed the feat/hud-customizer-v1 branch 2 times, most recently from 547facb to a7e6827 Compare August 20, 2024 15:29
Comment on lines 67 to 69
function parsePx(str: string): number | undefined {
if (str && str.slice(-2) === 'px') {
return Number.parseFloat(str.slice(0, -2));
}
return undefined;
}
Copy link
Member

Choose a reason for hiding this comment

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

Don't love nested functions like this, just as easy to read if parsePx is separate, and theoretically this is slower since parsePx captures panel (though in practice in V8 it's probably completely fine).

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

scripts/hud/customizer.ts Outdated Show resolved Hide resolved
@wertiop121 wertiop121 force-pushed the feat/hud-customizer-v1 branch from a7e6827 to bfbf0b0 Compare August 20, 2024 16:09
@wertiop121 wertiop121 force-pushed the feat/hud-customizer-v1 branch from bfbf0b0 to e0d1539 Compare August 20, 2024 16:10
@tsa96 tsa96 merged commit 927efbd into momentum-mod:feat/hud-customizer-v1 Sep 27, 2024
1 check passed
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.

2 participants