Skip to content

Conversation

OmerCD
Copy link
Contributor

@OmerCD OmerCD commented Aug 5, 2025

Brief Description of What This PR Does

This PR adds a new theme to compound text when it is in compact view.
image

I moved the Compound related files under gui directory to its own folder to make it look more organized.

I also added a simple functionality to clear selection in cell editor with right click which is really unrelated but annoyed me.

Related Issues
Closes #6234
Progress Checklist

Note: before starting this checklist the PR should be marked as non-draft.

  • PR author has checked that this PR works as intended and doesn't
    break existing features:
    https://wiki.revolutionarygamesstudio.com/wiki/Testing_Checklist
    (this is important as to not waste the time of Thrive team
    members reviewing this PR)
  • Initial code review passed (this and further items should not be checked by the PR author)
  • Functionality is confirmed working by another person (see above checklist link)
  • Final code review is passed and code conforms to the
    styleguide.

Before merging all CI jobs should finish on this PR without errors, if
there are automatically detected style issues they should be fixed by
the PR author. Merging must follow our
styleguide.

@revolutionary-bot
Copy link

Thank you for contributing to Thrive.

Before your contribution can be accepted, you need to sign our CLA.
You can sign it, and find more info about it, here: https://dev.revolutionarygamesstudio.com/cla

Once your contribution has been accepted you can ask to be included in the credits (https://wiki.revolutionarygamesstudio.com/wiki/Team_Members) and you can apply to join the team and use this work as a sample: https://revolutionarygamesstudio.com/application/

@hhyyrylainen
Copy link
Member

I also added a simple functionality to clear selection in cell editor with right click which is really unrelated but annoyed me.

Could you please make that as a separate PR because that is too unrelated to the other changes?

@OmerCD OmerCD force-pushed the compound-number-visibility-fix branch from 69a8ac4 to 9b54be8 Compare August 5, 2025 16:11
@hhyyrylainen
Copy link
Member

I see two architecture things I'd like to see done differently:

  • I'd prefer a solution where individual theme elements would be changed or the label font settings would be changed directly (so instead of swapping a theme, as that is a thing that is not used anywhere in the Thrive GUI currently)
  • And I don't really see the point of putting 2 scene files in their own subfolder. With that logic we would end up with half the number of current scene files as separate folders, and I think that is going to be a bigger mess.

So could you please rework those? Or let me know what I missed in my analysis and why the current approach is better.

@hhyyrylainen hhyyrylainen added this to the Release 0.8.3 milestone Aug 5, 2025
@OmerCD OmerCD force-pushed the compound-number-visibility-fix branch 2 times, most recently from 58dcd61 to 52043fc Compare August 5, 2025 16:48
@OmerCD
Copy link
Contributor Author

OmerCD commented Aug 5, 2025

Thanks for the quick feedback. I suspected things were out of place but with your feedback I hope it will be proper this time. I still find setting all options at once approach cleaner and revised my PR according to it.

@OmerCD OmerCD force-pushed the compound-number-visibility-fix branch 3 times, most recently from 4abc5a4 to 8a512ad Compare August 5, 2025 17:22
@OmerCD OmerCD force-pushed the compound-number-visibility-fix branch from 8a512ad to 134333c Compare August 5, 2025 17:34
Copy link
Member

@hhyyrylainen hhyyrylainen left a comment

Choose a reason for hiding this comment

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

The code looks a lot cleaner now. I still have a couple of comments about how to make it more flexible and to avoid extra small object allocations (which is something we try to avoid whenever possible during gameplay).

Refactored "normal" StyleBox name.
@OmerCD
Copy link
Contributor Author

OmerCD commented Aug 6, 2025

Thanks again for the insightful suggestions. I don't know very much about Godot and appreciate it very much. I am pushing the new changes and hopefully should be good enough.

@OmerCD OmerCD requested a review from hhyyrylainen August 6, 2025 23:39
@OmerCD
Copy link
Contributor Author

OmerCD commented Aug 7, 2025

Sorry to bother you with these little code pieces. I hope I will understand the codebase better for future PRs.

@OmerCD OmerCD requested a review from hhyyrylainen August 7, 2025 10:24
@hhyyrylainen
Copy link
Member

The code change looks perfect now, but I'll test gameplay functionality a bit later before merging.

Copy link
Member

@hhyyrylainen hhyyrylainen left a comment

Choose a reason for hiding this comment

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

I've now tested this, and unfortunately I don't think this looks very good...

kuva

That's what I see. The darkened area does not match the bar whatsoever, which does not look good in my eyes. So it needs tweaking. Or maybe my original idea about switching the font on the label to one with a shadow behind it would be a more viable approach?

@hhyyrylainen
Copy link
Member

If you don't mind I could try finishing this with the font shadow approach today so that this can be included as a fix in 0.8.3 release.

@hhyyrylainen
Copy link
Member

This is how my attempt looks:

kuva

I think it is good enough to ensure the visibility without causing much noticeable visual change. Most players probably won't notice the text gaining / losing the one pixel shadow.

@revolutionary-bot
Copy link

We are currently in feature freeze until the next release.
If your PR is not just a simple fix, then it may take until the release to get reviewed and merged.

Copy link
Member

@hhyyrylainen hhyyrylainen left a comment

Choose a reason for hiding this comment

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

I adjusted the visuals and the approach a bit, and I think this is now good. I'll merge this now for this to make it into the next release.

@hhyyrylainen hhyyrylainen merged commit 83d90a2 into Revolutionary-Games:master Aug 14, 2025
3 of 4 checks passed
@github-project-automation github-project-automation bot moved this from In progress to Done in Thrive Planning Aug 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Compound numbers blend into their icon bars in the compact view mode
3 participants