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

style(UI): Reskin the dice tool #1519

Merged
merged 23 commits into from
Jan 3, 2025
Merged

Conversation

rexy712
Copy link
Contributor

@rexy712 rexy712 commented Nov 24, 2024

This is an overhaul of the visuals of the dice tool.

Last Results Section

The top of the tool window is dedicated to showing the previous roll, being likely the most important thing one would want to see. The result is displayed clearly on the lefthand side in a prominent black-bordered box and the breakdown is to its right. Each Part of the result is in a separate block with operators floating between. Each Part's block shows a full breakdown of what the rolled values were.

History

The next part down is the history drawer. It is closed by default and can be opened by hitting the 'Full History +' toggle. The drawer starts small and grows up to a maximum size as it becomes populated by entries. Each entry represents a dice roll that the player did or was shared with them by another. An entry shows who performed it, what dice were rolled, and the grand total. Clicking on an entry opens a breakdown view with more details about what each dice's result was just like the breakdown view for the last results section. Just above the breakdown is a Reroll button. Clicking this will populate the input field with the input value that generated this roll to allow a player to quickly generate a result from the same roll string.

Options Cluster

The options cluster is pretty straight forward. On the left is the control for using 3D dice. No and Yes are identical to the way the old dice tool worked. 'Box' will add a dice box to the top of the tool window. 3D dice will be thrown inside of this box instead of on top of the gameboard when this option is enabled. This prevents issues like #1061 and is more aesthetically pleasing in my opinion.

On the right of the options cluster is the share toggle, which is identical to the old dice tool. Nothing to add here.

Advanced Options

The advanced options are the same as in the old dice tool layout, but they are rendered more as inside a drawer to visually distinguish what is a part of the 'advanced' menu. These options are also no longer between the literal digits and the rest of the input buttons, which felt a bit weird.

Literal and Dice Selection

The only change here is that the Operator and Literal ClickGroups have been combined together. I felt this was better looking and took up less space than having them separate. Other than that, these are the same.

Input, Roll, Reroll

Lastly, we have the input field. It's a bit more styled, but behaves the same for the most part. The only real difference is that it now scrolls to the end as you hit the on screen buttons so you can see what you're entering. Not a big difference, but nice.

The roll button is also just slightly styled with no other change. It now does a press animation when clicked and becomes disabled when the input box is empty.

The reroll button is a new addition. It simply populates the input field with the input string of the roll in the Last Results section. It overwrites anything in the input box, so be careful. This along with the reroll button in history was a feature requested in the discord, though there isn't a github issue to link.

Known Issues

Right now I can point to one major implementation problem. I tried to make babylonjs work with me, but never could fully succeed. The current method of switching between 3D dice in the dice box and on the game board is to generate 2 separate DiceThrowers, one for each. Then keep them cached and switch between them as requested. This is not ideal. The other option I tried out was to have only one engine loaded at a time and whenever a switch to the other canvas was requested, dispose of the old engine and generate a new one from scratch. This caused stuttering whenever a new engine loaded, which was also not ideal.

I saw info on having multiple render targets on the babylonjs docs, but there were some issues with implementing that solution that I couldn't get past. The first time rendering to a canvas, the geometry was slightly off which caused the dice to not behave properly. Closing the view and reopening it fixed the issue, but I don't know why.

With the current 2 engine setup, this issue doesn't stop the tool from working, it just can cause the page to become a bit heavier than is necessary. I did find that switching between the 2 canvases as the engine was loading would cause the engine creation to fail, so I made it so while either engine is loading, the toggle switch is temporarily disabled. The load takes less than 1 second, so shouldn't be an issue.

Another minor issue is the lack of i18n. I thought about following @SuikaXhq's lead and adding the i18n strings, but I know I couldn't do it as well as they did in #1518. I can barely come up with a good name for a variable let alone that many! I can try doing it if so requested.

Demo

dice-tool.mp4

@SuikaXhq
Copy link
Contributor

i18n names I made are actually a bit casual as long as they are clear enough. I can try to replace your labels into i18n strings and submit a PR to you in soon.

Copy link
Owner

@Kruptein Kruptein left a comment

Choose a reason for hiding this comment

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

Mostly small fluff.

I did check it out locally however and the 3d dice: On version, just gives me a big white screen. For some reason the background of the main babylon canvas is no longer transparent.

client/src/game/systems/dice/environment.ts Outdated Show resolved Hide resolved
client/src/game/systems/dice/index.ts Outdated Show resolved Hide resolved
Comment on lines +18 to +19
// const limitOperatorOptionNames = [t('game.ui.tools.DiceTool.limit_operator_options.keep'), t('game.ui.tools.DiceTool.limit_operator_options.drop'), t('game.ui.tools.DiceTool.limit_operator_options.min'), t('game.ui.tools.DiceTool.limit_operator_options.max')]
// const selectorOptionNames = ["=", ">", "<", t('game.ui.tools.DiceTool.selection_option_names.highest'), t('game.ui.tools.DiceTool.selection_option_names.lowest')]
Copy link
Owner

Choose a reason for hiding this comment

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

What's the deal with these?

Copy link
Contributor

Choose a reason for hiding this comment

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

These should be the i18n names in ClickGroup. I commented instead of removing them since you have said there would be an update on it to have the same API as ToggleGroup (like {label: string; value: T}) and they could be put into use then.

client/src/game/ui/tools/DiceTool.vue Outdated Show resolved Hide resolved
client/src/game/ui/tools/DiceTool.vue Outdated Show resolved Hide resolved
</div>
<div v-else-if="part.longResult" class="dice-result">
<div class="input">{{ part.input ?? "" }}</div>
<div class="ops">{{ '(' + part.longResult.replaceAll(',', ' + ') + ')' }}</div>
Copy link
Owner

Choose a reason for hiding this comment

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

I think it would be nice to add an extra option "Output" with options "short" and "long" or "expanded" or something. Which would toggle the visibility of this element.

Some people care about seeing the individual results, others just want the results and don't like the clutter of all those individual items. Having a simple toggle that can be toggled if you do need the details can be interesting I think. (And at all times have a hover message when you hover over a result show the details as well)

We can make the dice UI element a bit wider to fit 1 third toggle in that options row.

It might also be interesting to make this a markdown aware field so that the various notations for rolls that were kept or dropped also work. (See systems/chat/md.ts for an example, we only need a very basic markdown instance without special setups)

Copy link
Contributor Author

@rexy712 rexy712 Nov 30, 2024

Choose a reason for hiding this comment

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

I think it would be nice to add an extra option "Output" with options "short" and "long" or "expanded" or something. Which would toggle the visibility of this element.

I agree that giving the option is a nice idea. I really don't want to increase the width of the tool if I can avoid it. Right now the width is set to nicely fit the literal+symbol clickgroup, which looks good. I also don't want to increase height because it can get pretty tall with mutiple drawers open already.

I think some sort of options menu might be nice for this. Keep things that will only be changed infrequently hidden most of the time to cut down on clutter. In this menu, have separate options for last roll breakdown details and history item breakdown details. This also makes the tool more expandable in the future if more options are added. Obviously keep 3D dice and Share on the main screen since they're going to be used quite often.

It might also be interesting to make this a markdown aware field so that the various notations for rolls that were kept or dropped also work.

Somehow I didn't even realize the notation was supposed to be markdown lol. I could get that working.

client/src/game/ui/tools/DiceTool.vue Outdated Show resolved Hide resolved
Comment on lines 261 to 271
<div v-if="part.longResult" class="dice-result">
<div class="input">{{ part.input ?? "" }}</div>
<div class="ops">{{ '(' + part.longResult.replaceAll(',', ' + ') + ')' }}</div>
<div class="value">{{ part.shortResult }}</div>
</div>
<div v-else-if="part.shortResult === '+' || part.shortResult === '-'" class="operator-result">
{{ part.shortResult }}
</div>
<div v-else class="literal-result">
{{ part.shortResult }}
</div>
Copy link
Owner

Choose a reason for hiding this comment

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

This is completely repeated from the main result box if I'm not mistaken.

Might be worth to move this out to a separate component then (e.g. DiceResults.vue)

@rexy712
Copy link
Contributor Author

rexy712 commented Nov 30, 2024

I did check it out locally however and the 3d dice: On version, just gives me a big white screen. For some reason the background of the main babylon canvas is no longer transparent.

Hm that's weird and concerning. I don't see this behavior in chromium or firefox. I made a fresh clone of the repo to make sure it wasn't something lingering in my working copy, and I don't see the behavior there either. It should still clear to transparent with the line scene.clearColor = new Color4(0, 0, 0, 0);

Is it a white background since connecting to a game or only after loading the 3D dice?

@Kruptein
Copy link
Owner

I did check it out locally however and the 3d dice: On version, just gives me a big white screen. For some reason the background of the main babylon canvas is no longer transparent.

Hm that's weird and concerning. I don't see this behavior in chromium or firefox. I made a fresh clone of the repo to make sure it wasn't something lingering in my working copy, and I don't see the behavior there either. It should still clear to transparent with the line scene.clearColor = new Color4(0, 0, 0, 0);

Is it a white background since connecting to a game or only after loading the 3D dice?

Only after checking the 3d and the box variant worked fine. I checked the clearcolor and that was indeed still fine so unsure.
If you can't reproduce it I'll try to figure it out

@SuikaXhq
Copy link
Contributor

I did check it out locally however and the 3d dice: On version, just gives me a big white screen. For some reason the background of the main babylon canvas is no longer transparent.

Tested at commit 5e37a9d, didn't encounter that problem.

@Kruptein
Copy link
Owner

Kruptein commented Dec 4, 2024

Ok I figured it out. I only could reproduce it on my windows desktop at first and not on my linux laptop, so I thought some strange OS thing.

But it's because on my desktop I have a big screen, which ends up causing the camera's height to end up above the roof of the dicebox, because the camera's position is calculated based on the canvas height.

@rexy712
Copy link
Contributor Author

rexy712 commented Dec 4, 2024

But it's because on my desktop I have a big screen, which ends up causing the camera's height to end up above the roof of the dicebox, because the camera's position is calculated based on the canvas height.

Ah I see. I don't really like the camera height being based off one dimension of the canvas like that anyway. I think I'll just make them 2 constants. I was trying to be more clever than was warranted.

December is my busy season so progress will be slower. I'll still work on it when I have time.

@Kruptein
Copy link
Owner

Kruptein commented Dec 4, 2024

December is my busy season so progress will be slower. I'll still work on it when I have time.

No problem, thanks for taking the time in the first place!

@Kruptein Kruptein merged commit 4de6803 into Kruptein:dev Jan 3, 2025
6 checks 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.

3 participants