rojo-rbx / rojo

Rojo enables Roblox developers to use professional-grade software engineering tools
https://rojo.space
Mozilla Public License 2.0
908 stars 172 forks source link

warning "value cannot be converted to a number" on Billboard Gui's "MaxDistance" property #881

Open PhantomHaxxor opened 4 months ago

PhantomHaxxor commented 4 months ago

I have attached a Repro File (.zip). It contains a billboard gui located in ReplicatedStorage

Repro Version: Rojo 7.4.1. Repro Steps:

  1. run rojo build
  2. run rojo serve
  3. sync with Studio Plugin
value cannot be converted to a number  -  Edit
Stack Begin  -  Studio
Script 'user_RojoManagedPlugin.rbxm.Rojo.Packages.RbxDom.PropertyDescriptor', Line 19 - function set  -  Studio
Script 'user_RojoManagedPlugin.rbxm.Rojo.Packages.RbxDom.PropertyDescriptor', Line 77 - function write  -  Studio
Script 'user_RojoManagedPlugin.rbxm.Rojo.Plugin.Reconciler.setProperty', Line 31 - function setProperty  -  Studio
Script 'user_RojoManagedPlugin.rbxm.Rojo.Plugin.Reconciler.applyPatch', Line 204 - function applyPatch  -  Studio
Script 'user_RojoManagedPlugin.rbxm.Rojo.Plugin.Reconciler', Line 67 - function applyPatch  -  Studio
Script 'user_RojoManagedPlugin.rbxm.Rojo.Plugin.ServeSession', Line 285  -  Studio
Script 'user_RojoManagedPlugin.rbxm.Rojo.Packages.Promise', Line 172 - function runExecutor  -  Studio
Script 'user_RojoManagedPlugin.rbxm.Rojo.Packages.Promise', Line 181  -  Studio
Stack End  -  Studio
kennethloeffler commented 4 months ago

The cause of this is ultimately #363, but the warning only shows up now because of a change in rbx-dom: https://github.com/rojo-rbx/rbx-dom/pull/392.

The MaxDistance property in the provided model file is set to inf. JSON (the format that Rojo uses for transport) cannot represent infinities, and ends up encoded in JSON like this:

{ "Float32": null }

Roblox's HttpService:JSONDecode decodes this into an empty table, which after the change to rbx-dom I mentioned, causes the warning.

To properly fix this, we'll need to move away from JSON. I guess we'll slot this into the 7.5 roadmap...