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

Water current visuals #5922

Open
wants to merge 59 commits into
base: master
Choose a base branch
from
Open

Water current visuals #5922

wants to merge 59 commits into from

Conversation

dligr
Copy link
Member

@dligr dligr commented Feb 20, 2025

Brief Description of What This PR Does

Implements water current visuals using GPU particles. Also makes currents differ per patch and affect cells.

Current demonstration
Godot.Engine.2025.02.20.-.19.28.39.01.mp4

Related Issues

A roadmap item (https://wiki.revolutionarygamesstudio.com/wiki/Release_Roadmap#0.8.x_(world_and_auto-evo_updates))

Closes #4739 (no longer necessary as the current system now uses noise textures)

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.

@dligr dligr added this to the Release 0.8.2 milestone Feb 20, 2025
@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.

@hhyyrylainen
Copy link
Member

GPU particles seems pretty bad in terms of performance in general in Godot. Might be better to switch to CPU particles but that might not also perform that well. In the worst case we need some kind of custom visuals shader on a plane to represent the currents (which kind of was my initial guess as to how to implement the visuals).

Also I won't review this until after the 0.8.1 release as I simply don't have time.

@Patryk26g
Copy link
Contributor

Looks great! But I think it would be good idea to add a little bit of variation in the size of these particles. And I think that the currents should stop when you pause the game

@dligr
Copy link
Member Author

dligr commented Feb 21, 2025

GPU particles seems pretty bad in terms of performance in general in Godot. Might be better to switch to CPU particles but that might not also perform that well. In the worst case we need some kind of custom visuals shader on a plane to represent the currents (which kind of was my initial guess as to how to implement the visuals).

CPU particles might be unusable here, because they don't support particle shaders, which I used to apply currents to particles.

It's also possible to slightly optimize GPU particles by setting their FPS to a smaller value, but turning on interpolation, thus avoiding some calculations.

@HyperbolicHadron
Copy link
Contributor

The lines/square pattern seems to be significantly reduced now, though not entirely gone:
Screenshot from 2025-03-05 14-53-02

I think it is not too visually distracting now if it can't be fixed entirely.

At the beginning of the game, when the compounds just start to be mixed, it is most prominent:
Screenshot from 2025-03-06 10-54-31
Though it usually smooths out more after the initial movement to look more natural.

@hhyyrylainen
Copy link
Member

The technical implementation side is now very close to perfect. Though hopefully Godot 4.4 doesn't break anything with this.


if (!PauseManager.Instance.Paused)
{
material.SetShaderParameter(gameTimeParameterName, time);
Copy link
Member

Choose a reason for hiding this comment

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

Is it intentional that this passes in the old time? I think I'll swap this around the way I think this should be.

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 fixed the technical code style problems, but now when I playtested for the first time I noticed two graphical problems.

There's these really ugly lines that show up in the compound clouds:

2025-03-10_11 24 23 4929

I think those need to be solved. Also the clouds seem to be mixed up a lot more than before so maybe the cloud system should scale back how much currents it applies as now the clouds are a really mixed blob, more than before.

And one minor graphical thing I noticed is that when riding a current the particles in your "current" are jittery. This is probably the classical problem of the particles updating on each frame, but the player position updates on only each game simulation update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

Test if FastNoiseLite would be faster in the C++ module
5 participants