Skip to content

Conversation

@Bilal-Ghareeb
Copy link

@Bilal-Ghareeb Bilal-Ghareeb commented Nov 21, 2025

Summary

This PR fixes issue #3321 where Granitos would sink into the ground if a rock was dropped on top of them.

Changes

  • Commit 1: Prevented rocks from propagating movement into Granitos.
  • Commit 2: Fixed player + rock stacking case by refining rock-on-top tracking.
  • Commit 3: Fixed carried Granito + rock case by ejecting the small Granito and blocking propagation when carried.
  • Commit 4: Cleanup of redundant state resets after eject.

Reproduction Steps

  1. Spawn a Granito and drop a rock on top.
    • Before fix: Granito sinks into the ground.
    • After fix: Granito remains stable.
  2. Place a player standing on top of a Granito while the rock is on top of the Granito.
    • Before fix: Granito sinks again.
    • After fix: Granito remains stable.
  3. Let a big Granito carry a small Granito, then drop a rock on the small one.
    • Before fix: both sink into the ground.
    • After fix: small Granito ejects cleanly, big Granito remains stable.

Expected Behavior

  • Rocks no longer cause Granitos to sink in any stacking or carrying scenario.
  • Carried Granitos eject safely when hit by rocks.
  • Normal Granito + rock interactions remain unchanged.

Issue Reference

Closes #3321

Demo Video

Fix.mp4

Granito’s collision handling was updated to prevent rocks from propagating
movement into the granito, which previously caused granito to be forced
downward when a rock landed on its head. A new check ensures that rocks
do not trigger movement propagation.

- Added rock-specific guard in Granito::collision to block propagation

Closes SuperTux#3321
After the initial fix for rocks pushing granito into the floor, a new bug
appeared when a rock remained on granito’s head and the player jumped on
top. Granito would forget the rock and begin sinking again.

To resolve this, a new state flag (m_has_rock_on_top) was introduced and
checked in both collision and update. Granito now consistently tracks
whether a rock is present above its head across frames, preventing the
sinking bug while still allowing players to ride granito correctly.

- Added m_has_rock_on_top flag to Granito

- Updated collision to set flag when rock detected

- Added update logic to probe bounding box and clear flag when rock removed
When a big granito carried a smaller granito, dropping a rock on the
smaller one caused both to sink into the ground. This happened because
rock-on-top logic was applied even while the granito was in a carried
state, leaking movement propagation into the carrier.

To resolve this, collision handling was updated to eject the small
granito when a rock lands while it is carried, reset its physics, and
restore it to a standing state. This ensures rocks no longer propagate
movement through carried granitos and prevents both entities from
sinking.

- Added eject + reset logic when rock hits a carried granito

- Ensured carried granito transitions back to STATE_STAND

- Blocked propagation when rock is present on carried granito
Cleaned up collision handling by removing unnecessary state assignments
(STAND and original state) after ejecting a carried granito. The eject
logic already resets physics and restores a neutral state, so these
extra lines were redundant.

- Deleted redundant m_state/m_original_state assignments

- Deleted redundant set_action("stand", m_dir) call
@Bilal-Ghareeb Bilal-Ghareeb changed the title Fix Granito sinking when rock lands while carried (#3321) This PR fixes issue #3321 where Granitos would sink into the ground if a rock was dropped on top of them. Nov 21, 2025
@Bilal-Ghareeb Bilal-Ghareeb changed the title This PR fixes issue #3321 where Granitos would sink into the ground if a rock was dropped on top of them. Fix issue #3321 where Granitos would sink into the ground if a rock was dropped on top of them. Nov 21, 2025
- Use const reference in Rock iteration loop

- Remove newline in collision check
@tobbi
Copy link
Member

tobbi commented Nov 21, 2025

Thank you! I've triggered another build.

@Bilal-Ghareeb
Copy link
Author

Thank you! I've triggered another build.

Hi!

All checks have passed — is there anything else I should update before this can be merged ?

Also regarding the original issue: a comment mentioned that lanterns might have the same bug, but I tested them and they seem to behave correctly.

@tobbi
Copy link
Member

tobbi commented Nov 24, 2025

Thank you! I've triggered another build.

Hi!

All checks have passed — is there anything else I should update before this can be merged ?

Also regarding the original issue: a comment mentioned that lanterns might have the same bug, but I tested them and they seem to behave correctly.

I want someone else to at least review this as well before merging.

Copy link
Member

@Hypernoot Hypernoot left a comment

Choose a reason for hiding this comment

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

Tested in game, works.

Copy link
Member

@Frostwithasideofsalt Frostwithasideofsalt left a comment

Choose a reason for hiding this comment

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

Walking granitos turn into standing granitos if a rock is placed on them, this shouldn't happen as it can softlock levels.

Prevent walking granitos from turning into standing when a rock is placed on top

- Removed update logic that forced STATE_STAND when m_has_rock_on_top cleared
- Changed collision handling: small granitos now enter STATE_LOOKUP when a rock lands
- Added m_has_rock_on_top flag, treated like m_stepped_on, to restore original state cleanly
- Ensures walking granitos no longer softlock into standing state under rocks
Override Granitobig collision handling to separate it from small granitos

- Added virtual HitResponse collision(MovingObject&, const CollisionHit&) override
- Prevented GranitoBig from triggering small granitos' collision logic
- Makes big vs small granito collision paths cleaner and easier to debug
Granitobig propagate movement to objects on top

- In update(), added m_col.propagate_movement(m_physic.get_movement(dt_sec))
- Ensures rocks, lanterns, and players ride correctly on big granitos
- Matches expected behavior: big granitos carry objects above them
Rock abort collision handling when colliding with GranitoBig

- Added dynamic_cast check in Rock::collision
- Return ABORT_MOVE when colliding with GranitoBig
- Calls collision() but skips handling, as per collision_hit.hpp comment
- Allows rocks to propagate with big granitos without sinking them
@Bilal-Ghareeb
Copy link
Author

Bilal-Ghareeb commented Nov 27, 2025

Walking granitos turn into standing granitos if a rock is placed on them, this shouldn't happen as it can softlock levels.

Thanks for pointing this out I have adjusted the behaviour so walking granitos no longer incorrectly switch to standing when a rock is placed on them.

  • For small granitos, rocks now trigger the same “lookup” state as when a player stands on them. They don’t propagate movement upward, so they won’t softlock into standing anymore.

  • For big granitos, I separated their collision handling into their own override. Big granitos now propagate their movement to anything on top (player, rocks, lanterns), which matches expected platform‑like behavior.

  • Rocks colliding with big granitos return ABORT_MOVE, so the collision is called but no extra handling is applied this prevents sinking while still allowing propagation.

Copy link
Member

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

Frankly, speaking, what happens when you drop a rock on other objects? Is this a granito specific issue?

@swagtoy
Copy link
Member

swagtoy commented Nov 27, 2025

The logic for a rock being dropped on the second granito doesn't seem right anyway. I feel like we shouldn't mess with the sprite state. It looks weird.

EDIT: This is what @Frostwithasideofsalt was claiming

@Bilal-Ghareeb
Copy link
Author

Bilal-Ghareeb commented Nov 27, 2025

The logic for a rock being dropped on the second granito doesn't seem right anyway. I feel like we shouldn't mess with the sprite state. It looks weird.

EDIT: This is what @Frostwithasideofsalt was claiming

Just to clarify your comment when you say “the second granito” are you referring to:

the small granito,

the big granito,

or the case where a small granito is riding on top of a big granito (being carried)?

I want to make sure I understand the context correctly before adjusting the behaviour.

Fix code style in rock.cpp

- Rename granitoBig to granito to match the dynamic cast in granito.cpp
- Add a space after the If ")" and "{"
- Make sure to follow the style of lines 222->225 in rock.cpp
Remove redundant m_has_rock_on_top flag

- Rename m_stepped_on → m_has_entity_on_top to better reflect usage
- m_has_entity_on_top now indicates whether any entity (player or rock) was on top of the granito in the last frame
@Bilal-Ghareeb
Copy link
Author

Bilal-Ghareeb commented Nov 27, 2025

I’ve committed the requested style fixes and also unified the flags (m_stepped_on → m_has_entity_on_top) as mentioned.

Regarding this comment:

“The logic for a rock being dropped on the second granito doesn't seem right anyway. I feel like we shouldn't mess with the sprite state. It looks weird.”

This part still remains, and I need to understand it a bit more before I can adjust the behaviour properly.

Apologies if my work has required extra requests this is my first open source contribution, so I’m still learning the workflow. Any further feedback is appreciated.

Update declaration of 'granito' in rock.cpp to use a pointer-to-const

- Resolves the cppcheck style warning
src/object/rock.cpp:227:10: style: Variable 'granito' can be declared as pointer to const [constVariablePointer]

- Improves const correctness by ensuring 'granito' is only used for
read-only access, preventing accidental modification of the underlying object
and aligning with project coding standards.
@Bilal-Ghareeb Bilal-Ghareeb force-pushed the fix-rocks-granito-collision branch from 2273d79 to cd564ee Compare November 29, 2025 14:29
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.

[Bug]: Rocks push granitos into the floor

5 participants