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

Core - Fix load/save of deleted entities such as bushes and trees #153

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

Conversation

gnif
Copy link

@gnif gnif commented Jan 18, 2025

This resolves #152, aka the "Wrong parameter value" error mentioned in #35. Patches contain comments to explain the exact cause and resolution of the issue.

gnif added 2 commits January 18, 2025 20:29
EntityIDs are constructed from Low and high 32-bit IDs, the original code here didn't account for entities that have a high ID of zero, and values that were less then 8 characters long.

This new implementation removes the float math and uses bitwise operations to correctly convert to the expected array format in the correct byte order with no length limitations for the input.
Map entities such as trees and bushes do not have a high ID. This results in the game engine returning `0x0x` prefixes when ToString is called.

ToInt and FromString have also been updated to use a high ID of 0 when the array only contains a single element.
Copy link
Member

@Kexanone Kexanone left a comment

Choose a reason for hiding this comment

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

Thanks for the investigation and fix. Especially the 0x0x prefix makes me suspect that there's a bug in the pointer.ToString method on Linux. I'll probably open a ticket for it. For now we gonna have to handle it anyway. Btw. don't forget to add yourself as contributor to the AUTHORS file.

@gnif gnif requested a review from Kexanone January 18, 2025 11:20
@Kexanone Kexanone changed the title Fix load/save of deleted entities such as bushes and trees Core - Fix load/save of deleted entities such as bushes and trees Jan 18, 2025
@Kexanone Kexanone added the kind/bug Release Notes: **FIXED:** label Jan 18, 2025
@Kexanone Kexanone added this to the 1.2.1 milestone Jan 18, 2025
Copy link
Member

@Kexanone Kexanone left a comment

Choose a reason for hiding this comment

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

We have to think about how to handle 0x(nil) {}, which is what you get on Linux for the null pointer.

@gnif
Copy link
Author

gnif commented Jan 18, 2025

We have to think about how to handle 0x(nil) {}, which is what you get on Linux for the null pointer.

Since implementing these fixes I have not seen this again, I am pretty sure it was happening because the old HexStringToInt was returning an empty array, which was then treated as EntitiID.INVALID, resulting in the nil value.

@Kexanone
Copy link
Member

True, but I would argue that ACE_EntityIdHelper should nevertheless be able to handle EntitiID.INVALID as well.

@gnif
Copy link
Author

gnif commented Jan 19, 2025

Perhaps in FromString check for 0x(nil) and return EntityId.INVALID if we see it?

@Kexanone
Copy link
Member

Or we write it as 0x0 when we see it in ToString

@gnif
Copy link
Author

gnif commented Jan 19, 2025

I think we should do both actually, people upgrading with existing saves will have files full of 0x(nil) values to be cleaned up.

@Kexanone
Copy link
Member

Also handle the unprefixed version (nil) in ToString in case BI decides to fix it after all.

@gnif
Copy link
Author

gnif commented Jan 19, 2025

Done, and included handling of invalid IDs when loading the save. Note I have not tested this latest patch, I am away from my dev system atm.

@gnif
Copy link
Author

gnif commented Jan 20, 2025

Just FYI, I have now tested this and everything is working as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Release Notes: **FIXED:**
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Core - Wrong parameter value on Linux Dedicated
2 participants