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

Incorrect int handling in Enum class #888

Open
googleben opened this issue Oct 17, 2023 · 2 comments · May be fixed by #902
Open

Incorrect int handling in Enum class #888

googleben opened this issue Oct 17, 2023 · 2 comments · May be fixed by #902
Labels
bug Something isn't working

Comments

@googleben
Copy link

Bug in Enum class

Operating system: Windows
Game version: 2.01
CET version: 1.27.1
GPU: RTX 4080

The Problem

In the CET Enum class, there's some missed casts that result in enums not working correctly in Lua. I'll use EDeviceStatus for example. EDeviceStatus is a 32-bit enum, as defined by CEnum.actualSize which is 4 in this case. However, all the values for enums are stored as 64-bit int64_ts in the field CEnum.valueList (and CEnum.aliasValueList), type DynArray<int64_t> (see here) which means we need to do some type casting pretty much any time we interact with those fields. Looking at the code for the CET Enum class, we can see this at least seems to be done correctly in the get and set methods, but isn't done in any other method.

Take for example EDeviceStatus.UNPOWERED, which is -1 or 4294967295 if unsigned. You can check that this is the correct value by finding an unpowered door in the game and running this Lua code:

print(Game.GetTargetingSystem():GetLookAtObject(Game.GetPlayer(), false, false):GetDevicePS():GetDeviceState())

That code prints "EDeviceStatus : (4294967295)" - but the name for the state is missing. The name should be found by Enum.GetValueName, but it isn't, because of this line. I checked to make sure and, yes, the value for UNPOWERED is stored in memory as 0xFFFF FFFF FFFF FFFF (-1) - but we're comparing it to static_cast<int64_t>(m_value), converting from unsigned to signed - and when m_value was set on this line, -1 was converted to 4294967295, and the compiler doesn't know that m_value is a signed number anymore. And since -1 == 4294967295 is false, we can't find the correct name for the value.

This also affects the other incorrect methods in Enum, so for example in Lua print(EDeviceStatus.UNPOWERED) prints EDeviceStatus : UNPOWERED (18446744073709551615) because we incorrectly convert a 64-bit signed number to a 64-bit unsigned number, ignoring the fact that it's meant to be signed and 32-bit (the method in question here is Enum.SetValueByName).

The Solution

There are a few possible solutions here. First off we could save a lot of headache by sticking with the convention RED4Ext uses, which is to simply only use int64_ts for enums. I don't think that has any real downsides and means we don't have to do any casting in the code anymore.

If we'd prefer to remain faithful to CEnum.actualSize, we could use a union with all the possible sizes as fields and then pick the correct one using CEnum.actualSize whenever we need to - perhaps even create an enum for the sizes to make it more clear what's going on.

Finally, we could continue doing things the way we are, and just add switches in every method to handle the type conversion. I definitely don't think that's the right way to go but it's worth mentioning.

What I Can Do

If a code owner makes a decision on the approach to use, I'd be happy to handle writing the code and put a PR in. I was planning on adding some extension with things like iterators over game types and niceties like that anyways, so this would be a good small bug to get me started. Otherwise if there's any more information needed I'm happy to help.

@WSSDude WSSDude added the bug Something isn't working label Oct 25, 2023
@WSSDude
Copy link
Collaborator

WSSDude commented Oct 25, 2023

I believe it may actually be an union in their code, at least we do something similar at our work. But it would require proper runtime cast via static_cast based on the actualSize, not just cutting bytes. That may lead to issues in case they used negative values for example if we were just cutting bytes.

I suppose they also save if it is signed/unsigned along with size, otherwise whatever they do would end up error prone if they use both kinds of ints (unless they used something which somehow inlined this information and it is not kept in the class, which I doubt if they use mix of those by a chance, maybe I'm misunderstanding what you wrote...)

If you are willing to help with this, would be great! 🙇

@WSSDude
Copy link
Collaborator

WSSDude commented Oct 25, 2023

Btw, this must be the nicest written out issue we ever received 🥇

googleben added a commit to googleben/CyberEngineTweaks that referenced this issue Nov 9, 2023
googleben added a commit to googleben/CyberEngineTweaks that referenced this issue Nov 9, 2023
@googleben googleben linked a pull request Nov 9, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants