Page MenuHomeWildfire Games

Stringify failed type conversation
Needs RevisionPublic

Authored by vladislavbelov on May 1 2017, 2:34 PM.

Details

Reviewers
leper
elexis
Summary

Allow Stringify to convert JS objects for debug output.

Test Plan

Build, test e.g. with a random map using arrays of entities instead of entity strings.

Event Timeline

FeXoR created this revision.May 1 2017, 2:34 PM
FeXoR added a comment.May 1 2017, 3:26 PM
In D401#16690, @elexis wrote:

I guess that's just on the JS side?

Vulcan added a subscriber: Vulcan.May 1 2017, 5:08 PM

Build has FAILED

Link to build: http://jw:8080/job/phabricator/955/
See console output for more information: http://jw:8080/job/phabricator/955/console

FeXoR edited the summary of this revision. (Show Details)May 7 2017, 5:38 PM
vladislavbelov commandeered this revision.Nov 5 2017, 9:57 PM
vladislavbelov added a reviewer: FeXoR.
Vulcan added a comment.Nov 5 2017, 9:58 PM

Build failure - The Moirai have given mortals hearts that can endure.

vladislavbelov edited the summary of this revision. (Show Details)Nov 5 2017, 9:58 PM
elexis added a comment.EditedNov 5 2017, 10:17 PM

That's also hitting GUI and JS Simulation, so we should be really sure that this is wanted in all case.
What about an error stack? In #3965 we had this conversion error without any indication where it occured and had to put prints all over the engine code to locate it.
The #define miight be nicer to read with newlines.

I'm dubious about always stringifying this - try stringifying one or another massive object and it will segfault instead of printing the error (At least I always got them when stringifying too big stuff from JS)!

To me it's a won't fix, the feature doesn't seem to add something useful while adding disadvantages.

  • JSON.Stringify won't work for functions
  • This string can be indefinitely long, it can be very noisy.
  • The error should (and does) show the value type, but the actual value shouldn't matter.
  • In most cases we can reproduce the error (simulation, rmgen), so we can display the value if really needed. In the GUI we have to start investigating, most often its only undefined property. Then we don't need the patch either.
elexis requested changes to this revision.Sep 2 2018, 2:10 PM

Patch as is not acceptable as mentioned above, because JSON.stringify does break for many values (undefined, Set, functions), because too long values are too noisy. What the user really needs is a stacktrace where the error occurs rather than trying to guess from the value what might go on. Notice that if the same conversion function is called in JS and warns, we get a JS stacktrace, but if this function is called from C++ we don't.

This revision now requires changes to proceed.Sep 2 2018, 2:10 PM
FeXoR removed a reviewer: FeXoR.Nov 17 2018, 2:42 AM