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

Tux emits bubbles while being underwater #3014

Merged
merged 13 commits into from
Oct 20, 2024
Merged

Conversation

bruhmoent
Copy link
Member

@bruhmoent bruhmoent commented Jul 13, 2024

This pr adds a visual effect - when Tux is underwater, he will emit bubble particles that will follow a sine pattern until they reach the water surface.

Make sure to test.

@Brockengespenst
Copy link
Contributor

I think this is a pretty nice feature, but is it possible that the air_bubble-*.png files are missing?
Maybe some idea: What about using std::array for m_bubble_particles? This would make it easier to iterate over the array or do a check for bounds (e.g. when picking a random bubble).

@bruhmoent
Copy link
Member Author

Oh you're right, I forgot to push the bubbles.

@Brockengespenst
Copy link
Contributor

Looks pretty nice so far. Some comments from my side:

  • Sometimes it seems the bubbles are not emitted at the correct place after Tux has turned around. That looks a bit weird when the bubbles are emitted at the wrong side of Tux ;-)
  • I am not sure if the bubbles should really leave the water and vanish in the air. Probably they should start to vanish a bit earlier

@bruhmoent
Copy link
Member Author

I like the effect of them "popping" in the air. And for the bubbles not being emitted at the right place, I'm aware - but I don't know what to do about it - sometimes the sprite rotation gets registered improperly - At-least that's what I've observed.

@Brockengespenst
Copy link
Contributor

Some suggestions:

  • Use m_swimming_angle as it is already in radians
  • I do not fully understand your direction check. If comparing the angle in radians, I thinks something like m_swimming_angle > M_PI_2 && m_swimming_angle < M_PI_2 * 3.0 should check if Tux is facing left, all other cases Tux is facing right
  • If the beak was at Tux.height / 2, there would even be no need to check for direction, just rotate it. But if doing so in this case, this might lead to a small offset, maybe you can check if this looks still OK
  • I would not use std::vector for m_active_bubbles as it might lead to lots of copy operations when deleting entries as vector must guarantee that all entries are placed in a linear order on the heap. When deleting items in the middle, they have to be copied to a new place in linear order. As you do not need to access the bubles by index, you could simply use a std::list or similar

@bruhmoent
Copy link
Member Author

Thanks for the suggestions!

@Brockengespenst
Copy link
Contributor

Brockengespenst commented Jul 13, 2024

No problem. I really like this feature, but I think there are two small tweaks to be done:

  • I guess small and big Tux need to be handled differently. Small Tux swimming left looks good, but when small Tux swims right, there is a bit of space between the beak and the point where bubbles are emitted
  • Also, kind of related probably, left and right direction of Tux seem to need different offsets. Big Tux swimming right emits bubbles direcly at his beak, but big Tux swimming left emits the bubbles somwhere "behind his head"

It would be great if you could have a look at these two things.

Copy link
Member

@tobbi tobbi left a comment

Choose a reason for hiding this comment

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

Approve on account of code style. Haven't done any actual testing in-game, though.

@Rusty-Box
Copy link
Member

@bruhmoent Why did you not use the .sprite file I send you?

@Rusty-Box
Copy link
Member

Also, I think the bubbles vertical speed could be increased by quite a bit

@bruhmoent
Copy link
Member Author

@bruhmoent Why did you not use the .sprite file I send you?

I just used the images - I found the .sprite file to be unnecessary? Idk how does it matter really

Copy link
Member

@MatusGuy MatusGuy left a comment

Choose a reason for hiding this comment

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

Replace all glm::vec2s with Vectors

Despite what I mentioned. It looks great! Thanks for your contribution!

glm::vec2 beak_position;

// Determine direction based on the radians
if (m_swimming_angle > static_cast<float>(M_PI_2) && m_swimming_angle < 3.0f * static_cast<float>(M_PI_2)) // Facing left
Copy link
Member

Choose a reason for hiding this comment

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

We use math::PI_2

Suggested change
if (m_swimming_angle > static_cast<float>(M_PI_2) && m_swimming_angle < 3.0f * static_cast<float>(M_PI_2)) // Facing left
if (m_swimming_angle > static_cast<float>(math::PI_2)) && m_swimming_angle < 3.0f * static_cast<float>(math::PI_2)) // Facing left

m_bubble_particles[1] = Surface::from_file("images/particles/air_bubble-2.png");
m_bubble_particles[2] = Surface::from_file("images/particles/air_bubble-3.png");
m_bubble_particles[3] = Surface::from_file("images/particles/air_bubble-4.png");
m_bubble_timer.start(3.0f + (rand() % 2));
Copy link
Member

Choose a reason for hiding this comment

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

For usage of rng in graphics effects, use graphicsRandom

Suggested change
m_bubble_timer.start(3.0f + (rand() % 2));
m_bubble_timer.start(3.0f + graphicsRandom.randf(2));

src/object/player.hpp Outdated Show resolved Hide resolved
SurfacePtr bubble_surface = m_bubble_particles[random_index];

glm::vec2 bubble_pos;
if (m_swimming_angle > static_cast<float>(M_PI_2) && m_swimming_angle < 3.0f * static_cast<float>(M_PI_2)) // Facing left
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (m_swimming_angle > static_cast<float>(M_PI_2) && m_swimming_angle < 3.0f * static_cast<float>(M_PI_2)) // Facing left
if (m_swimming_angle > static_cast<float>(math::PI_2) && m_swimming_angle < 3.0f * static_cast<float>(math::PI_2)) // Facing left

Comment on lines 257 to 260
m_bubble_particles[0] = Surface::from_file("images/particles/air_bubble-1.png");
m_bubble_particles[1] = Surface::from_file("images/particles/air_bubble-2.png");
m_bubble_particles[2] = Surface::from_file("images/particles/air_bubble-3.png");
m_bubble_particles[3] = Surface::from_file("images/particles/air_bubble-4.png");
Copy link
Member

Choose a reason for hiding this comment

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

Apparently rusty sent you a sprite file? Use this in junction with the get_actions method

@Brockengespenst
Copy link
Contributor

Brockengespenst commented Jul 14, 2024

Sorry, but I have to slightly update my suggestion regarding how to determine the direction. From the swim sprite logic, I saw that the following code is used:
std::abs(m_swimming_angle) <= math::PI_2 ? Direction::RIGHT : Direction::LEFT
This seems to work more reliable, because it takes also negative angles into account.

@bruhmoent bruhmoent requested a review from MatusGuy July 16, 2024 12:31
Copy link
Member

@MatusGuy MatusGuy left a comment

Choose a reason for hiding this comment

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

Code looks good, will test later

Vector beak_position;

// Determine direction based on the radians
if (m_swimming_angle > static_cast<float>(math::PI_2) && m_swimming_angle < 3.0f * static_cast<float>(math::PI_2)) // Facing left
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please change the check to std::abs(m_swimming_angle) <= math::PI_2? This is used here in Player.cpp already, so the code would be consistent. Remember, that this condition checks for swimming right, so your conditional code must be swapped.

SurfacePtr bubble_surface = surfaces.at(random_index);

Vector bubble_pos(0.f, 0.f);
if (m_swimming_angle > static_cast<float>(math::PI_2) && m_swimming_angle < 3.0f * static_cast<float>(math::PI_2)) // Facing left
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please change the check to std::abs(m_swimming_angle) <= math::PI_2? This is used here in Player.cpp already, so the code would be consistent. Remember, that this condition checks for swimming right, so your conditional code must be swapped.

@bruhmoent bruhmoent requested a review from MatusGuy July 17, 2024 18:38
Copy link
Contributor

@Brockengespenst Brockengespenst left a comment

Choose a reason for hiding this comment

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

There is just this one include, that is obsolete again. Rest looks fine, thanks for adapting the left/right check.

src/object/player.hpp Show resolved Hide resolved
Copy link
Contributor

@Brockengespenst Brockengespenst left a comment

Choose a reason for hiding this comment

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

Looks fine to me.

@mstoeckl
Copy link
Contributor

While this looks nice and cartoony, I (random passerby) should warn that cartoons are often wrong, and actual penguins probably don't breathe out underwater. (Nor should any species that can't breathe in underwater, except if very close to the surface :-)

Instead of emitting periodic bubbles from their beaks, penguins might produce them through:

  1. splashing near water surface
  2. possible: some bubbles when opening beak underwater to grab things for the first time
  3. releasing trapped air after diving in (looks like many small trailing bubbles from the back/wings of penguin; could take some time for all bubbles to go.)

Therefore: I would suggest emitting the bubbles more irregularly in space and time; from the main body instead of from the beak; and maybe progressively reduce the bubble rate the longer Tux stays underwater.

@bruhmoent
Copy link
Member Author

SuperTux is not aimed to replicate realistic behaviours. This is just a little visual effect. This is out of scope for this PR - and this might be adjusted in the future.

@mstoeckl
Copy link
Contributor

SuperTux is not aimed to replicate realistic behaviours. This is just a little visual effect. This is out of scope for this PR - and this might be adjusted in the future.

Agreed. While I think foundations in reality are nice to have, in the end art and gameplay are what matter.

Copy link
Member

@MatusGuy MatusGuy left a comment

Choose a reason for hiding this comment

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

I can't confirm if the item in m_active_bubbles gets popped when the bubble goes offscreen, but make sure it does.

Almost there!

src/object/player.cpp Outdated Show resolved Hide resolved
src/object/player.cpp Show resolved Hide resolved
@Rusty-Box
Copy link
Member

The bubbles are still not animated despite the sprite file having 2 action of animation. One for bigger bubbles and one for smaller ones.

Copy link

@swagtoy swagtoy left a comment

Choose a reason for hiding this comment

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

You may be able to get away string_view instead of arrays of strings

Copy link
Member

@MatusGuy MatusGuy left a comment

Choose a reason for hiding this comment

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

Nice work

@MatusGuy MatusGuy merged commit 062dc80 into SuperTux:master Oct 20, 2024
33 of 35 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.

8 participants