Skip to content

Conversation

@Dynline
Copy link

@Dynline Dynline commented Mar 27, 2025

Brief Description of What This PR Does

Adds a canvas item shader to the icon used in the ChemicalEquation.tscn that makes it rotate.

Related Issues

closes #5023

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/

@Dynline Dynline changed the title #5023 spinner shader added spinner shader to chemical equation icon Mar 27, 2025
@Dynline Dynline changed the title added spinner shader to chemical equation icon Added spinner shader to chemical equation icon Mar 27, 2025
@hhyyrylainen
Copy link
Member

Thanks for taking the time to help out with Thrive development.
I noticed the CLA check is failing. Was there a problem with signing the CLA or did you just not get around to it yet? I ask because there's been some recent problems with the CLA signature detection not working correctly.

@Dynline
Copy link
Author

Dynline commented Mar 28, 2025

Thanks for taking the time to help out with Thrive development. I noticed the CLA check is failing. Was there a problem with signing the CLA or did you just not get around to it yet? I ask because there's been some recent problems with the CLA signature detection not working correctly.

Hi! Yes, I'm having problems signing it, I actually planned to ask about it in the forum. I tried a few times, but I get this error:

Error, server responded with: {"type":"https://tools.ietf.org/html/rfc9110#section-15.6.1","title":"An error occurred while processing your request.","status":500,"detail":"Failed to upload signed document to remote storage. Please try again.","traceId":"00-071bad2311f82778246098db00c7a441-f9e04ec63450d331-00"}, InternalServerError

Same error today.

@hhyyrylainen
Copy link
Member

I see. I checked the server logs and yeah there's a problem uploading the signatures to storage. I did a few small changes to the process that maybe did something (Revolutionary-Games/RevolutionaryWebApp@0e81629) and restarted the server. I just tried signing the CLA again myself and that seemed to work now.

So could you please retry the CLA signing now? It should hopefully work without issue this time.

@hhyyrylainen hhyyrylainen added this to the Release 0.8.2 milestone Mar 28, 2025
@Dynline Dynline marked this pull request as ready for review March 28, 2025 09:55
@Dynline
Copy link
Author

Dynline commented Mar 28, 2025

I see. I checked the server logs and yeah there's a problem uploading the signatures to storage. I did a few small changes to the process that maybe did something (Revolutionary-Games/RevolutionaryWebApp@0e81629) and restarted the server. I just tried signing the CLA again myself and that seemed to work now.

So could you please retry the CLA signing now? It should hopefully work without issue this time.

Thanks for checking that! It works now.

@hhyyrylainen hhyyrylainen requested review from a team March 28, 2025 09:58
Copy link
Member

@hhyyrylainen hhyyrylainen Mar 28, 2025

Choose a reason for hiding this comment

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

I think for clarity it would be better to split this file into two: the more hands off pixel variant and the current vertex variant that needs an explicit size.

Also could this shader be also applied to the loading screen? As I think that's the one other place that Thrive uses a spinner that this PR doesn't modify yet.

Copy link
Author

Choose a reason for hiding this comment

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

The shader is now splitted into two.

Also yes ! Just added it to the loading screen icon. It replaces the parts of code that were making it spin before.

I also wanted to ask about the image size of some icons, for example the one used for the chemical reaction is 800px scaled down to 32. Usually 800 for an icon is pretty big, do you need them to be this size for future parts of the game?

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. We've had issues in the past where there's an icon that's 24x24 but we need it higher resolution to not look blurry on a 4K screen. And as most artists on the team become inactive after a while, there's no way for us to get updated versions for the icons. So it is better to get such a big resolution that there's no way we'd ever need to redo the icon (rather than having icons that are 5 years old now but need to be redone for higher resolution).

Though, we are switching our icons over to SVG, but obviously that would be a ton of work to redo all old icons so icons are only slowly being replaced. SVGs don't have the same resolution issues so those are imported at smaller scale into the game (as it is very easy to just change the import scale later if required).

Copy link
Author

Choose a reason for hiding this comment

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

I see! thank you for letting me know that. I wanted to make sure before making any unwanted changes, as I plan to contribute some more shader or graphics related things.

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.

Testing in game, the loading screen spinner is good. So the shader works. However, the process speed icon (in ChemicalEquation) has two issues:

  • There is still the old code that tries to rotate things so when I removed the pivot offset (which should be unnecessary with these changes) there's now a bug where the texture rotates around its top left as well. So there's code that would need to be removed.
  • But also the process speed spinner must pause when the game is paused and this shader approach doesn't do that. So I think that what actually needs to be done is one more shader variant that allows manual control from the C# on the current rotation value. That way the C# code can still control the rotation amount and pause correctly but use the shader to apply the rotation.

I committed a few slight style fixes also to your branch, hope you don't mind.

@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.

@github-actions
Copy link

This PR has been inactive for a while and as such is being marked
stale. This PR will be automatically closed if this stays stale
for a while.

If this is still being worked on / will probably resume work at
some point please feel free to continue working on this and reopen
this in case this was already automatically closed.

@github-actions github-actions bot added the stale label Jul 30, 2025
@hhyyrylainen hhyyrylainen moved this from In progress to started but stuck (help wanted) in Thrive Planning Aug 8, 2025
@hhyyrylainen hhyyrylainen removed this from the Release 0.8.3 milestone Aug 8, 2025
@github-actions github-actions bot removed the stale label Aug 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: started but stuck (help wanted)

Development

Successfully merging this pull request may close these issues.

Implement 2D spinner shader as alternative to the removed pivots in Godot 4

3 participants