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

Replace huge advance value by a safer one with the same behavior #11

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AlexTMjugador
Copy link

@AlexTMjugador AlexTMjugador commented May 3, 2024

The default.json font file contains a font advance value of 1e400. This value is too large to be represented as-is even by IEEE 754 64-bit floating point numbers as used by Java and, by extension, Minecraft. Gson, the JSON deserialization library used by Minecraft, cannot properly represent such a number either, but the deserialization process is lenient and chugs along with a practically close enough value.

According to the ECMA JSON standard, numbers in JSON can be arbitrarily large, so such a number is technically within the specification. However, the standard points out that different programs may disagree on how to represent numbers, which can lead to interoperability problems in practice:

JSON is agnostic about numbers. In any programming language, there can be a variety of number types of
various capacities and complements, fixed or floating, binary or decimal. That can make interchange between
different programming languages difficult
. JSON instead offers only the representation of numbers that
humans use: a sequence of digits. All programming languages know how to make sense of digit sequences
even if they disagree on internal representations. That is enough to allow interchange.

On the other hand, IETF RFC 7159, another JSON standard, states:

This specification allows implementations to set limits on the range and precision of numbers accepted. Since software that implements IEEE 754-2008 binary64 (double precision) numbers [IEEE754] is generally available and widely used, good interoperability can be achieved by implementations that expect no more precision or range than these provide, in the sense that implementations will approximate JSON numbers within the expected precision. A JSON number such as 1E400 or 3.141592653589793238462643383279 may indicate potential interoperability problems, since it suggests that the software that created it expects receiving software to have greater capabilities for numeric magnitude and precision than is widely available.

Besides with Minecraft itself, one such interoperability problem occurs with PackSquash, which relies on serde_json for JSON parsing, and that parser (rightly) refuses to handle numbers that cannot be represented as floats.

To avoid interoperability problems, let's use an advance value of 1e37, which can be represented even by 32-bit floats and works just as well in practice as an "infinite" value.

The `default.json` font file contains a font advance value of `1e400`.
This value is too big to be represented as-is even by IEEE 754 64-bit
floating point numbers as used by Java and, by extension, Minecraft.
Gson, the JSON deserialization library used by Minecraft, cannot
properly represent such a number either, but the deserialization process
is lenient and chugs along with a practically close enough value.

According to the ECMA JSON standard, numbers in JSON may be arbitrarily
big, so such a number is technically within specification. However, the
standard highlights the possibility that different programs disagree on
the way they represent numbers, which in practice can cause
interoperability issues.

One such interoperability issue happens with PackSquash, which relies on
`serde_json` for JSON parsing, and that parser (reasonbly) refuses to
deal with numbers that cannot be represented as floats.

In order to avoid interoperability problems, let's use an advance value
of `1e37`, which can be represented even by 32-bit floats and works
equally well enough in practice as an "infinite" value.
@AmberWat
Copy link
Owner

AmberWat commented May 3, 2024

I'm well aware of the limitations of floating point numbers. The value is used precisely because it is translated into infinity by the game. Not just a huge number, but true infinity. Because of this, a change to a smaller value would introduce a change in behavior, which could be problematic for some users.

Interoperability has historically not been a big concern since the behavior of the game is fully known and the few tools I've encountered that deal with Minecraft fonts have not displayed problems with it.

I want to explore what can and should be done before merging this. For example, what if the value was isolated to its own file?

@AlexTMjugador
Copy link
Author

Thanks for taking a look at the PR! 😄

I'm well aware of the limitations of floating point numbers. The value is used precisely because it is translated into infinity by the game. Not just a huge number, but true infinity.

Yeah, I figured that such a huge value would be rounded to infinity by the game. Still, as outlined above, relying on a specific JSON parser behavior is hacky, and Minecraft has a history of updating and completely changing the libraries it uses to parse things. For example, as recently as the 1.20.5 release, the library used to read Ogg Vorbis files changed. While it's arguably less likely that the JSON parser will be significantly changed, given how widespread the overall reliance on Gson's lenient behavior is, I'd still consider it a real possibility.

Because of this, a change to a smaller value would introduce a change in behavior, which could be problematic for some users.

While this is technically true, I've been suggesting PackSquash users try replacing the problematic value with 1e37 for some time now, and I've never heard of it causing them any problems. Also, 1e37 is a value that can be considered practically infinite, as it's 1.22 x 1033 times the next largest width space of 8192. Even with very large screens and lots of characters, I have a hard time coming up with a realistic scenario where this huge value wouldn't behave the same as infinity. Not to mention that Minecraft probably wasn't designed with truly infinite advances in mind: it's kind of a happy coincidence that no math operations end up propagating infinities or NaN to unintended places.

I want to explore what can and should be done before merging this. For example, what if the value was isolated to its own file?

I'm down to explore stuff too! However, I'm afraid that isolating this value in a separate font JSON file would just move the problem to that file, not really fix it. On the other hand, changing serde_json on my end would range from having a negative performance impact to being impractical, and parsing some fields in PackSquash in a special way does not set a good precedent as I believe open source projects should strive to cooperate with each other to be technically excellent. Do you have other ideas?

@AmberWat
Copy link
Owner

AmberWat commented May 4, 2024

(I don't consider packs having a slim chance of breaking in future versions to be a concern. The resource pack format changes slightly every version anyway, making it an unwinnable battle. We now also have the option of presenting different versions of files to different versions of the game.)

If isolating it to it's own JSON file and setting it up to be ignored on your end isn't an option, I think I'd rather just take it out. I can put in a new character that is huge but not infinite.

@AlexTMjugador
Copy link
Author

AlexTMjugador commented May 4, 2024

(I don't consider packs having a slim chance of breaking in future versions to be a concern. The resource pack format changes slightly every version anyway, making it an unwinnable battle. We now also have the option of presenting different versions of files to different versions of the game.)

Fair enough. I'd still advocate trying to avoid dependencies on specific implementation details as much as possible to make life easier when upgrading, but I also understand that specific decisions on the matter mostly come down to personal considerations of workflow and perceived breakage probability and impact.

If isolating it to it's own JSON file and setting it up to be ignored on your end isn't an option, I think I'd rather just take it out. I can put in a new character that is huge but not infinite.

Yes, as far as I know that would be the best option. serde_json has a lossless number deserialization mode that can be enabled globally at compile time, but it is implemented by deserializing numbers to strings, which has a noticeable performance impact for common JSON-heavy resource packs, such as those containing many item models. While there are other potential workarounds, they are decidedly not pretty, and I'd prefer to avoid them if possible.

Nevertheless, despite my lack of evidence that an advance of 1e37 causes problems, I'm not entirely confident in asserting that such a change would completely lack significant user-visible side effects either, given my limited familiarity with the intricacies of this particular resource pack. I thought about potential side effects and could find none, but still.

Would it help if I reached out to affected users to gather explicit feedback on how an advancement of 1e37 fared for them? Please feel free to let me know if there's any way I can assist you further in nailing this down! 👍

RedVortexDev referenced this pull request in Tqlted/ACE-RP Oct 28, 2024
@AlexTMjugador
Copy link
Author

I was just reminded about this PR when a new person joined my Discord server and shared their wishes on seeing it merged… 😂

I'm curious whether there are any remaining tasks or changes needed here? Would it be enough to update the README.md to clarify that after this change these space characters won’t be truly infinite, just very large?

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.

2 participants