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

wrong export if enum as flags have more than 30 values #3658

Closed
ms502040 opened this issue Apr 14, 2023 · 6 comments · Fixed by #3666
Closed

wrong export if enum as flags have more than 30 values #3658

ms502040 opened this issue Apr 14, 2023 · 6 comments · Fixed by #3666
Labels
bug Broken behavior.

Comments

@ms502040
Copy link

Describe the bug
If add more than 30 values (31 or 32) to enum when checked "Allow multiple values" (act ass flags) exported to json values are wrong.
There is possible overflow on 30 bit.

Question
Why only 32 values if we have today 64 bit computers? Is there any masochist here who programs games on 32 bit windows? 😏

To Reproduce

  • create enum
  • check "Allow multiple values"
  • add 31 or 32 values
  • assign this enum to object
  • check some values
  • export to json
  • see exported file

Expected behavior
right export or unable to add more than 30 values as flags

Specifications:

  • OS: Windows 10
  • Tiled Version: 1.10.1
@ms502040 ms502040 added the bug Broken behavior. label Apr 14, 2023
@eishiya
Copy link
Contributor

eishiya commented Apr 14, 2023

When describing that something is wrong, you should really specify in what way it's wrong. In this case, the value is always output as 0, even if it has some flags set.

This issue affects TMX/TSX as well, not just JSON, and it affects saved files too, not just exports.

As for "why" - the choice to use 32-bit or 64-bit ints is not to do with architecture - 32-bit machines can work with 64-bit ints and vice versa. Rather, 32-bit ints are simply a very safe choice for bit fields, they are supported by the vast majority of programming and scripting languages - some scripting languages in particular only support floating points, and even when those have the same number of bits as a 64-bit int, they cannot represent the same data reliably. For example, in JavaScript, 0b1111111111111111111111111111111111111111111111111111111111111111 (64 1 bits) is decimal 18446744073709552000, which is not the same value you'd get if you interpreted that as an unsigned or signed 64-bit int. If you try to do bitwise operations on this value, or other values that have the highest 10 or so bits set, you will just get 0. This same number representation can represent and do bitwise operations on the full range of 32-bit integer values just fine, however.

@ms502040
Copy link
Author

output not always 0.

@eishiya
Copy link
Contributor

eishiya commented Apr 15, 2023

Oh, can you provide some examples then (inputs in Tiled vs output in the file)? When I tried it, I always got 0 in both JSON and XML. I didn't try every possible combination of flags, but I tried a bunch, including both the high and low bits.

@ms502040
Copy link
Author

Try add or remove 31 or 32 value when enum assigned to object. But I think it's unnecessary, because it's a overflow. Maybe there is signed integer type as storage. You have to look into the source code.

@eishiya
Copy link
Contributor

eishiya commented Apr 15, 2023

I did that, I always got 0, I never got results other than 0 except when I deleted the extra enum values so the total was under 31, and the enum worked correctly. Either way, the results are wrong, and you're right that we'd have to look into the code.

Unfortunately I got lost in my attempt xP It seems like when stored as numbers rather than strings, the value just gets passed along as QVariant (which should be able to losslessly hold that), but I am not sure what exactly is stored there at runtime, and why the highest two bits would cause problems. I'll have to leave this to someone who knows the properties code better than I do ):

@bjorn
Copy link
Member

bjorn commented Apr 15, 2023

I've done a short attempt at switching the stored value in the QVariant from int to uint, but it did not help. The QtFlagPropertyManager, which is used internally by the properties view for displaying and editing flag values, is also using int. This means the maximum supported flag value is 2147483647, which is just one below the value for the 31st flag (2147483648), so indeed only 30 flag values are supported at the moment.

I'm afraid that changing the code to support 32 flag values is going to be a little bit tricky (and would also mean introducing an unsigned int property type), so I'd first of all fix this by reducing the values limit applied by the UI from 32 to 30.

bjorn added a commit to bjorn/tiled-dev that referenced this issue Apr 18, 2023
Due to the use of a 32-bit int to store the flags, the maximum number of
flags is actually 30 rather than 32. When more entries are added, the
editing of enum values breaks entirely.

The translations have been updated to use a parameter for the maximum
number of flags in an enum, to make it easier to adjust this value when
it might be increased in the future.

Closes mapeditor#3658
bjorn added a commit to bjorn/tiled-dev that referenced this issue Apr 18, 2023
Due to the use of a 32-bit int to store the flags, the maximum number of
flags is actually 30 rather than 32. When more entries are added, the
editing of enum values breaks entirely.

The translations have been updated to use a parameter for the maximum
number of flags in an enum, to make it easier to adjust this value when
it might be increased in the future.

Closes mapeditor#3658
bjorn added a commit to bjorn/tiled-dev that referenced this issue Aug 4, 2023
Due to the use of a 32-bit int to store the flags, the maximum number of
flags is actually 30 rather than 32. When more entries are added, the
editing of enum values breaks entirely.

The translations have been updated to use a parameter for the maximum
number of flags in an enum, to make it easier to adjust this value when
it might be increased in the future.

Closes mapeditor#3658
bjorn added a commit to bjorn/tiled-dev that referenced this issue Aug 4, 2023
Due to the use of a 32-bit int to store the flags, the maximum number of
flags is actually 31 rather than 32. When more entries are added, the
editing of enum values breaks entirely.

The translations have been updated to use a parameter for the maximum
number of flags in an enum, to make it easier to adjust this value when
it might be increased in the future.

Closes mapeditor#3658
bjorn added a commit to bjorn/tiled-dev that referenced this issue Aug 4, 2023
Due to the use of a 32-bit int to store the flags, the maximum number of
flags is actually 31 rather than 32. When more entries are added, the
editing of enum values breaks entirely.

The translations have been updated to use a parameter for the maximum
number of flags in an enum, to make it easier to adjust this value when
it might be increased in the future.

Closes mapeditor#3658
bjorn added a commit that referenced this issue Aug 4, 2023
Due to the use of a 32-bit int to store the flags, the maximum number of
flags is actually 31 rather than 32. When more entries are added, the
editing of enum values breaks entirely.

The translations have been updated to use a parameter for the maximum
number of flags in an enum, to make it easier to adjust this value when
it might be increased in the future.

Closes #3658
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Broken behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants