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

fix(core/rdr3): AMV hardcoded limit causing blinking #3145

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

Conversation

alexguirre
Copy link

@alexguirre alexguirre commented Feb 9, 2025

Goal of this PR

There is a global bitset used to store the enabled/disabled state of every AMV. This bitset has a hardcoded size of 6016 entries, once there are more AMVs than the bitset size, it starts reading/writing out-of-bounds. This causes the AMVs to start blinking and can potentially crash.

How is this PR achieving the goal

This patch re-allocates that bitset with enough space to fit the capacity of all the AMV pools. The functions that access it are patched to point to the new buffer and use the new size constants.

This PR applies to the following area(s)

RedM

Successfully tested on

Game builds: 1491, 1436, 1355, 1311

Platforms: Windows

Checklist

  • Code compiles and has been tested successfully.
  • Code explains itself well and/or is documented.
  • My commit message explains what the changes do and what they are for.
  • No extra compilation warnings are added by these changes.

Fixes issues

Fixes #2821.

There is a global bitset used to store the enabled/disabled state of every AMV. This bitset has a hardcoded size
of 6016 entries, once there are more AMVs than the bitset size, it starts reading/writing out-of-bounds. This causes
the AMVs to start blinking and can potentially crash.

This patch replaces that bitset with a dynamic bitset resized to fit the capacity of all the AMV pools.
@github-actions github-actions bot added RedM Issues/PRs related to RedM invalid Requires changes before it's considered valid and can be (re)triaged labels Feb 9, 2025
@alexguirre
Copy link
Author

I simplified the patch by re-allocating the bitset buffer instead, should be less fragile than the stubs. Though now it hardcodes the AMV pool sizes, I didn't find any existing function to read the sizes from gameconfig and GetPoolBase is not initialized at that point. Let me know if there is a better way.

@github-actions github-actions bot added triage Needs a preliminary assessment to determine the urgency and required action invalid Requires changes before it's considered valid and can be (re)triaged and removed invalid Requires changes before it's considered valid and can be (re)triaged triage Needs a preliminary assessment to determine the urgency and required action labels Feb 10, 2025
@Ktos93
Copy link
Contributor

Ktos93 commented Feb 11, 2025

Patch tested, verified, and working as expected. All tests passed, and the build is successful. Ready to merge.
image

static HookFunction hookFunction([]()
{
// keep in sync with gameconfig
constexpr size_t CAmbientMaskVolume_PoolSize = 6686;
Copy link
Collaborator

@prikolium-cfx prikolium-cfx Feb 21, 2025

Choose a reason for hiding this comment

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

Currently we don't allow to increase these 3 pools and such way is safe, but what should we do in future when anybody need to extend one of these pools?

And what will happen if we define bigger value here than real pool size? Probably we can set here maximum possible value, for example double pool size, then it's fine.

Copy link
Author

Choose a reason for hiding this comment

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

Currently we don't allow to increase these 3 pools and such way is safe, but what should we do in future when anybody need to extend one of these pools?

In case of enabling increase_pool_size with these pools, looking up the increase values in fx::PoolSizeManager::GetIncreaseRequest() and adding them to these pool size constants would be enough. The increase request map is already initialized when this hook function runs, so should be straightforward to add.

And what will happen if we define bigger value here than real pool size? Probably we can set here maximum possible value, for example double pool size, then it's fine.

Nothing, just a bit of unused memory.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid Requires changes before it's considered valid and can be (re)triaged RedM Issues/PRs related to RedM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

After a certain amount of custom/edited AMV for MLO/Interiors, vehicles (wagons & trains) start to blink
3 participants