Skip to content

Conversation

mtxfellen
Copy link

@mtxfellen mtxfellen commented May 25, 2025

Description

The PR #618, merged in 52e568b happened to introduce a bug in CCurrencyPack::ComeToRest() that this PR aims to address:

  • Use PointIsWithin() instead of WorldSpaceSurroundingBounds() so that complex trigger shapes are accounted for.
    • We received some reports about money being auto-collected on some maps in large, perfectly accessible locations.

This PR also fixes some other minor bugs in this method:

  • Add missing early returns that could've rarely allowed currency to be auto-collected multiple times.
  • Prevent money being collected in disabled respawn rooms.
  • Move m_bDistributed check so that red money can be auto-collected if it drops outside the playable space.
  • Normalise GetCollisionOrigin() and GetAbsOrigin() usage as GetAbsOrigin().
    • They are the same for this entity.

Co-authored-by: John Kvalevog <jkvalevog@protonmail.com>
Vector vecMins, vecMaxs;
pRespawnRoom->GetCollideable()->WorldSpaceSurroundingBounds( &vecMins, &vecMaxs );
if ( IsPointInBox( GetCollideable()->GetCollisionOrigin(), vecMins, vecMaxs ) )
if ( !pRespawnRoom->m_bDisabled && pRespawnRoom->PointIsWithin( GetAbsOrigin() ) )
Copy link
Contributor

@FlaminSarge FlaminSarge May 26, 2025

Choose a reason for hiding this comment

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

If the respawn room gets disabled before the money comes to rest, will this result in the money not autocollecting? Some maps may disable certain respawn rooms for robots mid-game but may not make those areas walkable for players.

Copy link
Author

Choose a reason for hiding this comment

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

That would indeed happen, but I'm operating on the assumption that there's not really much reason to disable spawnroom entities in MvM except for making an area walkable and using RecomputeBlockers, given that robots won't attempt to hotswap their loadouts and their actual spawnpoints will continue to function as the mission maker directs independent of the spawnroom's status. Though of course if there's any maps this change would break, it would be worth reviewing this part again.

Vector vecMins, vecMaxs;
pRespawnRoom->GetCollideable()->WorldSpaceSurroundingBounds( &vecMins, &vecMaxs );
if ( IsPointInBox( GetCollideable()->GetCollisionOrigin(), vecMins, vecMaxs ) )
if ( !pRespawnRoom->m_bDisabled && pRespawnRoom->PointIsWithin( GetAbsOrigin() ) )
Copy link
Contributor

@Mentrillum Mentrillum Jul 3, 2025

Choose a reason for hiding this comment

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

Suggested change
if ( !pRespawnRoom->m_bDisabled && pRespawnRoom->PointIsWithin( GetAbsOrigin() ) )
if ( !pRespawnRoom->m_bDisabled && ( pRespawnRoom->PointIsWithin( GetAbsOrigin() ) || pRespawnRoom->IsTouching( this ) )

Correct me if I'm wrong but couldn't we also double check to see if the cash entity is touching the trigger using IsTouching?

Copy link
Author

Choose a reason for hiding this comment

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

IsTouching() would require the spawnroom to have spawnflags set that allow cash entities to register as touching to return true, but they usually only have SF_TRIGGER_ALLOW_CLIENTS set.

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.

4 participants