search

C7-Game / Prototype

An early-stage, open-source 4X strategy game
https://c7-game.github.io/
MIT License
32 stars 9 forks source link

Fixing Movement UI With Mixed Numbers #429

Closed benskywalker closed 9 months ago

benskywalker commented 9 months ago

Fixing issue #350 . This is another PR since I closed #428 due to a merge mistake on my end. This pr adds mixed numbers for remaining movement points. Feedback on whether this change is worthwhile is requested. @QuintillusCFC image

pcen commented 9 months ago

Looks good, thanks for working on this and congrats on your first PR in C7!!

nit on style: we use camelCase for variables so for consistency we should update the SO code too.

Also, are there other UI elements that will require nicely displaying floats in different formats? if so we could put the number formatting in a static class designed for formatting numbers, but I'm not sure if any other cases exist

benskywalker commented 9 months ago

Looks good, thanks for working on this and congrats on your first PR in C7!!

nit on style: we use camelCase for variables so for consistency we should update the SO code too.

Also, are there other UI elements that will require nicely displaying floats in different formats? if so we could put the number formatting in a static class designed for formatting numbers, but I'm not sure if any other cases exist

Thank you! Good points, I've refactored for camelCase. I can't think of any other uses for this, but I'm happy to move it if we can find one. It's kind of niche because it only applies when we need to display a decimal to users, AND when that decimal is intrinsically created by fractions. Maybe if we ever want to show Effective Combat Strength as a preview?

I've also fixed a bug where 0.999 would sometimes show as 99/100 or 999/1000. Now it should reliably show 1. I've also modified Seek as you suggested. @pcen , if you have the bandwidth it might be good if you could check out the branch to review it. My dev environment is still not completely stable, so a doublecheck might be nice

pcen commented 9 months ago

Something interesting I noticed is that after I cloned your repo, but before I opened the project in the Godot editor, just running it from the command line gave the following errors:

Screenshot 2023-09-23 at 2 25 30 AM

And when running, the lower right-hand corner UI box was empty for me:

Screenshot 2023-09-23 at 2 24 22 AM

After opening the project in the editor, then running it from either the command line or in the editor looks normal and doesn't give any errors. I'm curious as to what difference it makes opening the project in the editor (the git diff doesn't change, so whatever the diff is is gitignored...) But running it after I open it in the editor looks great!

benskywalker commented 9 months ago

That's odd, I've never tried running it from the command line before! I'll try that going forward

benskywalker commented 9 months ago

It looks like we're good to merge unless anyone has other concerns

pcen commented 9 months ago

@WildWeazel can we get another stamp before merging?

WildWeazel commented 9 months ago

I ran from the command line but had checked out this PR from my workspace, and did not see those errors. Seems like some kind of clean build issue. I had thought about where else this might apply, and could only come up with the right-click menu which is already covered. If it turns up elsewhere, it's easy enough to extract. Anyway, congrats on your first merge! That means I have a new task to do... update the credits!