Implement Font type #248

Corecii · 2023-01-15T17:51:21Z

This PR implements the Font type:

Implements binary de/serializer
Implements xml de/serializer
Implements Lua/json de/serializer
Adds some relevant tests
Updates reflection database

This is my first time working with rbx-dom's code, so I may have gotten some things wrong! I also have some general questions and concerns:

The Font value I saw in a hex editor looked different from what rbx-dom would output. Does rbx-dom do some transformation on all binary property data to make them sense? Maybe I didn't read deep enough about it. Should I have included information about that in my documentation of the Font type?
The Font type has a u8 and u16 which act a lot like enums but aren't exactly enums. I made special types for these with support for an Other so that any new items should just work even if they're not nicely represented. Was this the correct move? I could have represented Weight with just a u16, but Style needs a u8 <-> name converter so that it can be serialized to/from xml.
The FontStyle type serializes to xml by name, but to binary as u8. I included an Other enum variant here since I figured having support for unknown new values was a good thing, but that was before I saw how xml did it. This means unknown new FontStyles can go from binary -> binary, but not binary -> xml or xml -> anything. Is this okay, or do we want to remove the Other variant here? Related: in cases where the variant is unknown, I default to Normal. Would it be better to error?
The default font style I have listed is unstyled Source Sans Pro. This matches the default when you make a new object in Studio, but not the default when loading a type in Studio that's missing the Font property. Studio will actually give TextLabels (and such) that are missing the Font property and unstyled Arial font. Should that be our default, or should Source Sans Pro be our default?

Corecii · 2023-01-15T17:54:16Z

Oh, I just realized this is also missing conversion from the old font property to this new one, so if a very old rbxm of rbxl is loaded with rbx-dom, its FontFace will be incorrect. What's the correct way to handle that case?

Edit: I realize this isn't necessary to do. The object deserialized by rbx-dom will lack the new Font property, but that's fine because it will include the old font property which Roblox will convert to the new property fine. I was getting this confused with some other related issue caused by rbx-dom not having Font support.

Dekkonot

In a hurry at the moment so I can't do a more comprehensive review of the actual code, but here's one for the spec file at least.

It generally looks good, it just needs some changes to fit with the format we use throughout the spec files.

docs/binary.md

Co-authored-by: Micah <dekkonot@rocketmail.com>

Dekkonot

Gave the files a proper look!

This is mostly fine, but there's some cleanup I'd like done with FontWeight and FontStyle. We'd ideally not use .into() or have to clone them, and most of my comments are related to that + cleanup from changing them.

rbx_binary/src/deserializer/state.rs

rbx_binary/src/serializer/state.rs

rbx_binary/src/text_deserializer.rs

rbx_types/src/font.rs

rbx_xml/src/types/font.rs

rbx_binary/src/serializer/state.rs

* Use explicit from_x and to_x * Derive and use Copy * Nicer default when writing cached_face_id

Corecii · 2023-01-18T23:11:08Z

My most last commit addresses all of your comments!

Resolved

(replaced by ScreenInsets)

Corecii · 2023-01-19T00:04:25Z

Tests should pass now. It's unrelated to the font implementation -- probably related to updating the reflection database. Since IgnoreGuiInset no longer saves, it isn't present in the decoded or encoded rbxms.

Dekkonot

Looks good to me!

For anyone curious about the prior CI failure, the serialization of IgnoreGuiInserts was changed in release 554 and updating the reflection database exposed us to that change. Thus (as expected) the test involving ScreenGuis failed.

We likely should try to fix this in the future so that Roblox reflection changes don't interfere with our tests like this, but that's well outside of scope for this PR.

Dekkonot · 2023-01-19T00:30:50Z

I meant to leave a comment a while back answering your questions but I forgor and got caught in the review sauce, so you'll have to forgive me.

The Font value I saw in a hex editor looked different from what rbx-dom would output. Does rbx-dom do some transformation on all binary property data to make them sense? Maybe I didn't read deep enough about it. Should I have included information about that in my documentation of the Font type?

Roblox serializes their binary format with compression (every chunk is LZ4 compressed), but rbx-dom doesn't at the moment. That's the only difference I can think of.

The Font type has a u8 and u16 which act a lot like enums but aren't exactly enums. I made special types for these with support for an Other so that any new items should just work even if they're not nicely represented. Was this the correct move? I could have represented Weight with just a u16, but Style needs a u8 <-> name converter so that it can be serialized to/from xml.

I considered this while I was doing the review, and eventually decided that it was a good call. It doesn't really impact things for rbx-dom either way, but using enums should make it easier for Rojo to implement a good format for Font property types.

The FontStyle type serializes to xml by name, but to binary as u8. I included an Other enum variant here since I figured having support for unknown new values was a good thing, but that was before I saw how xml did it. This means unknown new FontStyles can go from binary -> binary, but not binary -> xml or xml -> anything. Is this okay, or do we want to remove the Other variant here? Related: in cases where the variant is unknown, I default to Normal. Would it be better to error?

A younger less experienced version of me would say "we should error whenever we encounter unknown data" but as I've seen rbx-dom and Rojo get surprised by things, I'm inclined to say it's not a bad idea to maintain data when we can. The XML limit is a shame but isn't our fault because there's quite literally nothing we could do about it either way. At the very least this implementation will let binary files do what they need to do.

The default font style I have listed is unstyled Source Sans Pro. This matches the default when you make a new object in Studio, but not the default when loading a type in Studio that's missing the Font property. Studio will actually give TextLabels (and such) that are missing the Font property and unstyled Arial font. Should that be our default, or should Source Sans Pro be our default?

Anybody who cares about Font will be overriding it so it isn't a big deal and I'd rather use what Roblox's default is because it's probably what people will be expecting anyway. Not sure if this is what we do everywhere but I wouldn't be surprised if the same reasoning was used.

LPGhatguy

Thank you for the PR! Here are a few comments from me, then this change looks good to me.

docs/binary.md

rbx_types/src/font.rs

rbx_xml/src/types/font.rs

LPGhatguy · 2023-01-20T23:16:17Z

The compatibility story for this property type might be a bit hairy. When merging older models without Font with newer models that do have the property, which property takes precedence in Studio? Will users need to re-save all of their models? Should we offer some sort of normalization or migration pass?

@Dekkonot @Corecii

Roblox serializes their binary format with compression (every chunk is LZ4 compressed), but rbx-dom doesn't at the moment. That's the only difference I can think of.

rbx_binary does output compressed chunks. The compression code is here:

rbx-dom/rbx_binary/src/chunk.rs

Lines 79 to 102 in 3a3563e

    
               pub fn dump<W: Write>(self, mut writer: W) -> io::Result<()> { 
        
                   writer.write_all(self.chunk_name)?; 
        
                   match self.compression { 
        
                       ChunkCompression::Compressed => { 
        
                           let compressed = lz4::block::compress(&self.buffer, None, false)?; 
        
                           writer.write_le_u32(compressed.len() as u32)?; 
        
                           writer.write_le_u32(self.buffer.len() as u32)?; 
        
                           writer.write_le_u32(0)?; 
        
                           writer.write_all(&compressed)?; 
        
                       } 
        
                       ChunkCompression::Uncompressed => { 
        
                           writer.write_le_u32(0)?; 
        
                           writer.write_le_u32(self.buffer.len() as u32)?; 
        
                           writer.write_le_u32(0)?; 
        
                           writer.write_all(&self.buffer)?; 
        
                       } 
        
                   } 
        
                   Ok(()) 
        
               }

and is referenced all over in the serializer:

rbx-dom/rbx_binary/src/serializer/state.rs

Line 443 in 3a3563e

let mut chunk = ChunkBuilder::new(b"SSTR", ChunkCompression::Compressed);

The default font style I have listed is unstyled Source Sans Pro. This matches the default when you make a new object in Studio, but not the default when loading a type in Studio that's missing the Font property. Studio will actually give TextLabels (and such) that are missing the Font property and unstyled Arial font. Should that be our default, or should Source Sans Pro be our default?

We should match the default value for what Roblox does for creating the instance with Instance.new, not the value when inserted via Studio's UI. That matches what happens when the Font property is unset, Arial. We shouldn't deviate from the strategy of using the instance's defaults instead of Studio's nice defaults since it can mask problems when things are broken. In any case, rbx_reflection should pick up the new property and property type and make the default value (Arial) explicit anyways.

Dekkonot · 2023-01-21T00:55:17Z

rbx_binary does output compressed chunks. The compression code is here:

That is rather embarrassing on my part! I thought I'd looked properly and I don't really have an excuse because I did in fact know that... I literally wrote the code for serializing SSTR. 🤦🏼 Thank you for the correction. In that case, I'm not sure how to explain the difference between rbx-dom's output and Studio's beyond maybe the compression level we use.

When merging older models without Font with newer models that do have the property, which property takes precedence in Studio? Will users need to re-save all of their models? Should we offer some sort of normalization or migration pass?

Every value of the Font enum has an equivalent Font value with the exception of Unknown, which doesn't do anything when Studio reads it. This means that we can directly translate the Font enum to Font values when we have to. This would have to be done via a migration pass, but it could be done in the background because it isn't destructive.

Corecii · 2023-01-21T21:57:06Z

I'm going to be away from home for the next week and may not have time to implement fixes it changes to this PR in that time. I'll get to it when I'm back, or maybe if I have some downtime while I'm gone! Just want to let you all know beforehand so you don't think I've forgotten or abandoned this PR.

LPGhatguy · 2023-01-21T22:11:39Z

No rush on addressing feedback! These days I'm not around consistently. I've been working on trying to get myself out of the pipeline and find groups of maintainers for all this stuff I'm still the main blocker on.

That is rather embarrassing on my part! I thought I'd looked properly and I don't really have an excuse because I did in fact know that... I literally wrote the code for serializing SSTR. 🤦🏼 Thank you for the correction. In that case, I'm not sure how to explain the difference between rbx-dom's output and Studio's beyond maybe the compression level we use.

I figure that's probably it. There are usually many ways to compress the same data, and LZ4 is tunable.

Every value of the Font enum has an equivalent Font value with the exception of Unknown, which doesn't do anything when Studio reads it. This means that we can directly translate the Font enum to Font values when we have to. This would have to be done via a migration pass, but it could be done in the background because it isn't destructive.

This is really good news. I wonder if this is something we should plan to do in rbx_binary if we get any user reports of weird behavior. Tools like Remodel and Rojo that aggregate models might produce some funny results if we don't do anything.

Dekkonot · 2023-01-23T01:55:35Z

This is really good news. I wonder if this is something we should plan to do in rbx_binary if we get any user reports of weird behavior. Tools like Remodel and Rojo that aggregate models might produce some funny results if we don't do anything.

If we do it in rbx_binary, we're essentially promising to do it a second time if anything like this happens again. I don't love that idea and would instead prefer we leave it up to consumers to patch files. This would let them decide how to handle it themselves and let them patch properties we don't/can't. However, if we do decide to implement it, I'd prefer we do it with a crate that wasn't rbx_binary.

The idea of a codec that modifies data in that fashion feels wrong even if the intent is to fix something Roblox did. It should be done by someone else, regardless of it it's another rbx_dom crate or an upstream user.

NecroticNanite · 2023-02-09T22:33:26Z

Just checking if anyone is still looking into this, and if it will be released soon?
This is going to be a big problem for us very soon if not resolved

Corecii · 2023-02-12T06:08:12Z

All of your feedback should be addressed now!

In any case, rbx_reflection should pick up the new property and property type and make the default value (Arial) explicit anyways.

Does this mean the default trait does not need to be changed to reflect Roblox? Should we still change it to match?

NecroticNanite · 2023-02-14T21:33:24Z

Hi, can someone explain how to link our local aftman.toml file to the WIP Uplift games? I tried a variety of URL formats and didn't have much success

Corecii · 2023-02-14T21:46:02Z

Hi, can someone explain how to link our local aftman.toml file to the WIP Uplift games? I tried a variety of URL formats and didn't have much success.

Our fork versions are one version ahead plus a prerelease tag. Here's what our rojo and remodel entries look like:

[tools]
rojo = "UpliftGames/rojo@7.2.2-uplift.release.4"
remodel = "UpliftGames/remodel@0.12.0-uplift.release.9"

Don't forget to switch back to rojo-rbx/rojo when the Font fixes are merged in to the main repo. The versions above are from the very first version of this PR -- which works and is compatible, but is outdated and is missing some non-critical fixes.

NecroticNanite · 2023-02-21T23:01:36Z

@Corecii I tested changing my aftman file to what you have above, but I still encounter the same issue where fonts are not synced
Output file has :
<Font name="FontFace"> <Family><url>rbxasset://fonts/families/GothamSSm.json</url></Family> <Weight>700</Weight> <Style>Normal</Style> </Font>
But I still get arial in studio after deleting and re-importing via Rojo

NecroticNanite · 2023-02-27T00:28:01Z

Sorry to bump again, but is there an update on when this might get merged? Or an answer to my above question as to why the forked version still exhibits the same issue?

Kampfkarren

Small nits

rbx_dom_lua/src/EncodedValue.lua

rbx_types/src/font.rs

Corecii · 2023-02-28T06:25:24Z

Nits fixed!

@NecroticNanite thanks for this report! rbx_dom had been set to exclude Font types from the reflection database and I didn't catch that when implementing. Additionally, we must manually include the new version of rbx_dom in the plugin in the rojo repo. We didn't catch this at Uplift because we're not syncing in models with rojo -- we only needed updated rbx_dom for remodel. The fork should work now if you change the version to rojo = "UpliftGames/rojo@7.2.2-uplift.release.5" and install the updated plugin through either the rojo menu in vs code or the rojo plugin install command.

NecroticNanite · 2023-03-02T05:03:07Z

Thanks for fixing that - however, I cannot download that version :(
Aftman gives me the following: "Aftman error: Could not find release UpliftGames/rojo@7.2.2-uplift.release.5"
Installing previous versions works fine (tried .2 and .4)
I'm guessing not published somehow?

Corecii · 2023-03-02T06:03:24Z

@NecroticNanite You're correct! Again, a symptom of us not needing this (yet). I had GitHub generate the release draft but didn't go press publish on it when it was done. Try now!

NecroticNanite · 2023-03-02T07:24:37Z

@Corecii Thanks a lot, that seems to have done the trick! Good timing too, we've got a tricky merge coming up and a release to sync to different environments :D

NecroticNanite · 2023-03-03T00:29:52Z

@Corecii Hi, we're just testing this now.
First issue:
Old files (from probably 6-12 months ago), that have not been re-exported, will prevent Rojo starting:
The file has a Font XML as follows:

<token name="Font">9</token>
<Font name="FontFace"></Font>

It will throw the following error when trying to start Rojo:

[ERROR rojo] Malformed rbxm file: [path]/[to]\Conversation.rbxmx
        
        Caused by:
            line 139, column 27: Unexpected XML event EndElement(Font)

Not sure how easy it will be to add back-compat for this - if it's not possible, will likely make this MR go from a minor hotfix into a full major version release, as anyone using it will have to do a fresh export of any model/rbxmx files.

The fix (at least, it seems to fix it) is to delete the blank lines from any rbxmx files - possibly the parser should just ignore those empty blocks?

(We're testing a few other things, and will send any issues along as/when we hit them)

Corecii · 2023-03-03T01:30:38Z

Oh that's fascinating! I'm sure we can just ignore it if it's empty. Do you happen to have any files you're willing to share which I can use to test a fix for this locally?

Edit: I realize I can easily make an example given the snippet you posted. Thanks!

I'm a little worried that there might be something similar in the binary rbxm though, and I'm not sure I have any models with this issue to test with. I'll just focus on fixing this issue with the xml for now.

NecroticNanite · 2023-03-03T01:40:05Z

Oh that's fascinating! I'm sure we can just ignore it if it's empty. Do you happen to have any files you're willing to share which I can use to test a fix for this locally?

Conversation.rbxmx - Pastbin: https://pastebin.com/qhAUyGzP
Password: xbL1NXXtUc

NecroticNanite · 2023-03-03T01:41:53Z

Everything else seems to be working great though!

LPGhatguy · 2023-03-12T21:05:20Z

Updated the MSRV on master, should fix the CI build.

Moving forward with the merge, plus follow-up changes

LPGhatguy

LGTM. I'm going to put in a follow-up commit to update the signatures of from_u8 and from_u16 for the sub-types of Font.

Looks like we forgot to do this in #248

Corecii and others added 6 commits January 12, 2023 16:30

Implement Font type

9752641

Error when not impl future FontStyles

c9a798f

Add docs

cca1027

Merge remote-tracking branch 'upstream/master' into implement-font-type

d345184

Delete old tests

5fd55f5

Add new test

5edf212

Corecii requested review from LPGhatguy, Dekkonot and Kampfkarren as code owners January 15, 2023 17:51

Dekkonot requested changes Jan 16, 2023

View reviewed changes

docs/binary.md Outdated Show resolved Hide resolved

docs/binary.md Outdated Show resolved Hide resolved

docs/binary.md Outdated Show resolved Hide resolved

Fix formatting consistency issues in spec file

ff6e9f1

Co-authored-by: Micah <dekkonot@rocketmail.com>

Dekkonot previously requested changes Jan 17, 2023

View reviewed changes

Kampfkarren reviewed Jan 17, 2023

View reviewed changes

rbx_binary/src/serializer/state.rs Outdated Show resolved Hide resolved

Kampfkarren mentioned this pull request Jan 17, 2023

TextLabel's Font is not exported rojo-rbx/remodel#92

Open

Address PR comments

575486e

* Use explicit from_x and to_x * Derive and use Copy * Nicer default when writing cached_face_id

IgnoreGuiInset no longer saves

2f9efd3

(replaced by ScreenInsets)

Dekkonot approved these changes Jan 19, 2023

View reviewed changes

LPGhatguy requested changes Jan 20, 2023

View reviewed changes

LPGhatguy mentioned this pull request Jan 20, 2023

Font Face doesn't sync correctly rojo-rbx/rojo#650

Closed

Remote Other for Font Weight and Style

881f4b5

Corecii added 2 commits February 13, 2023 11:10

Fix camelCase attribute on FontStyle

feed1e1

Add PR changes to CHANGELOG files

b25efb0

Kampfkarren previously requested changes Feb 27, 2023

View reviewed changes

rbx_dom_lua/src/EncodedValue.lua Outdated Show resolved Hide resolved

rbx_types/src/font.rs Show resolved Hide resolved

rbx_types/src/font.rs Outdated Show resolved Hide resolved

set added 5 commits February 28, 2023 00:06

Fix Font types from being excluded

36279e5

Update database to reflect Font inclusion

0df4bb7

Use if...else

4b162bb

Fix minor formatting

bf6b91e

to_x -> as_x

3760427

Corecii mentioned this pull request Mar 12, 2023

Add Property Migration #253

Closed

Merge remote-tracking branch 'upstream/master' into implement-font-type

8ea3f02

LPGhatguy approved these changes Mar 12, 2023

View reviewed changes

LPGhatguy merged commit 1227bfd into rojo-rbx:master Mar 12, 2023

kennethloeffler mentioned this pull request May 22, 2023

Add Font to property coverage table #272

Merged

Dekkonot pushed a commit that referenced this pull request May 22, 2023

Add Font to property coverage table (#272)

6bf7c10

Looks like we forgot to do this in #248

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Font type #248

Implement Font type #248

Corecii commented Jan 15, 2023

Corecii commented Jan 15, 2023 •

edited

Loading

Dekkonot left a comment

Dekkonot left a comment

Corecii commented Jan 18, 2023

Corecii commented Jan 19, 2023

Dekkonot left a comment

Dekkonot commented Jan 19, 2023

LPGhatguy left a comment

LPGhatguy commented Jan 20, 2023

Dekkonot commented Jan 21, 2023

Corecii commented Jan 21, 2023

LPGhatguy commented Jan 21, 2023

Dekkonot commented Jan 23, 2023

NecroticNanite commented Feb 9, 2023

Corecii commented Feb 12, 2023

NecroticNanite commented Feb 14, 2023

Corecii commented Feb 14, 2023

NecroticNanite commented Feb 21, 2023

NecroticNanite commented Feb 27, 2023

Kampfkarren left a comment

Corecii commented Feb 28, 2023

NecroticNanite commented Mar 2, 2023

Corecii commented Mar 2, 2023

NecroticNanite commented Mar 2, 2023

NecroticNanite commented Mar 3, 2023 •

edited

Loading

Corecii commented Mar 3, 2023 •

edited

Loading

NecroticNanite commented Mar 3, 2023 •

edited

Loading

NecroticNanite commented Mar 3, 2023

LPGhatguy commented Mar 12, 2023

LPGhatguy left a comment

Implement Font type #248

Implement Font type #248

Conversation

Corecii commented Jan 15, 2023

Corecii commented Jan 15, 2023 • edited Loading

Dekkonot left a comment

Choose a reason for hiding this comment

Dekkonot left a comment

Choose a reason for hiding this comment

Corecii commented Jan 18, 2023

Corecii commented Jan 19, 2023

Dekkonot left a comment

Choose a reason for hiding this comment

Dekkonot commented Jan 19, 2023

LPGhatguy left a comment

Choose a reason for hiding this comment

LPGhatguy commented Jan 20, 2023

Dekkonot commented Jan 21, 2023

Corecii commented Jan 21, 2023

LPGhatguy commented Jan 21, 2023

Dekkonot commented Jan 23, 2023

NecroticNanite commented Feb 9, 2023

Corecii commented Feb 12, 2023

NecroticNanite commented Feb 14, 2023

Corecii commented Feb 14, 2023

NecroticNanite commented Feb 21, 2023

NecroticNanite commented Feb 27, 2023

Kampfkarren left a comment

Choose a reason for hiding this comment

Corecii commented Feb 28, 2023

NecroticNanite commented Mar 2, 2023

Corecii commented Mar 2, 2023

NecroticNanite commented Mar 2, 2023

NecroticNanite commented Mar 3, 2023 • edited Loading

Corecii commented Mar 3, 2023 • edited Loading

NecroticNanite commented Mar 3, 2023 • edited Loading

NecroticNanite commented Mar 3, 2023

LPGhatguy commented Mar 12, 2023

LPGhatguy left a comment

Choose a reason for hiding this comment

Corecii commented Jan 15, 2023 •

edited

Loading

NecroticNanite commented Mar 3, 2023 •

edited

Loading

Corecii commented Mar 3, 2023 •

edited

Loading

NecroticNanite commented Mar 3, 2023 •

edited

Loading