Opened 6 years ago
Closed 5 years ago
#22253 closed defect (fixed)
Incorrect json representation for graphics
Reported by:  chapoton  Owned by:  

Priority:  major  Milestone:  sage7.6 
Component:  graphics  Keywords:  
Cc:  novoselt, egourgoulhon  Merged in:  
Authors:  Frédéric Chapoton  Reviewers:  Paul Masson 
Report Upstream:  N/A  Work issues:  
Branch:  fbd8b10 (Commits, GitHub, GitLab)  Commit:  fbd8b10344636dc09211356be7daaf2f1ff25f38 
Dependencies:  Stopgaps: 
Description (last modified by )
see https://groups.google.com/forum/#!topic/sagedevel/Wjo7NkmvCks
The problem lies in the file src/sage/plot/plot3d/index_face_set.pyx
, where the json is generated. There are two problems with the generated json:
 The quote signs used in the file are
'
and not"
as per [Json spec](http://www.ecmainternational.org/publications/files/ECMAST/ECMA404.pdf).  Every dictionary key is unquoted, which is incorrect. Thus, a point
{x:0,y:0,z:0}
should really be
{"x":0,"y":0,"z":0}
as per Json spec.
Attachments (1)
Change History (16)
comment:1 Changed 6 years ago by
 Branch set to u/chapoton/22253
 Commit set to 9a7269f0aa771d5a755384d65c4eee55adb90b0d
 Description modified (diff)
 Status changed from new to needs_review
comment:2 Changed 6 years ago by
 Commit changed from 9a7269f0aa771d5a755384d65c4eee55adb90b0d to 5f7e950acf177fb89023445f6e331861fd8dcfef
Branch pushed to git repo; I updated commit sha1. New commits:
5f7e950  trac 22253 fix doctests

comment:3 Changed 6 years ago by
 Commit changed from 5f7e950acf177fb89023445f6e331861fd8dcfef to 11e4447438ab44199fc3b0d36e95416a8e4459c4
Branch pushed to git repo; I updated commit sha1. New commits:
11e4447  trac 22253 fix more doctests

comment:4 Changed 6 years ago by
Seems ok with the canvas3d viewer (in sagenb):
sage: u,v = var('u,v') sage: def cf(u,v): return sin(u+v/2)**2 sage: P = parametric_plot3d((cos(u), sin(u)+cos(v), sin(v)), ....: (u,0,2*pi), (v,pi,pi), color=(cf,colormaps.PiYG), plot_points=[60,60]) sage: P.show(viewer='canvas3d')
Needs to be tested in other kinds of notebooks.
Ok also in jupyter notebook. Remains to check in cloud (not that easy).
comment:5 Changed 6 years ago by
 Cc novoselt egourgoulhon added
 Reviewers set to Paul Masson
All doctests pass in sage/plot/plot3d. Works with the new native Three.js viewer.
I would like to point out that this modification will bloat Three.js files without any change to the rendering, since JavaScript doesn't care whether array keys are quoted or not. Presumably that was part of the motivation for using formally incorrect JSON to begin with, so I didn't change it when writing the Three.js viewer.
comment:6 Changed 6 years ago by
 Commit changed from 11e4447438ab44199fc3b0d36e95416a8e4459c4 to d04e1b5ea463681f2f1288e600de3022381f96f6
comment:7 Changed 6 years ago by
one still needs to change
local/share/sage/ext/doctest/rich_output/example.canvas3d
but this is not under git control ?
Changed 6 years ago by
comment:8 Changed 6 years ago by
I have attached the modified example file. What now ?
comment:9 Changed 6 years ago by
 Commit changed from d04e1b5ea463681f2f1288e600de3022381f96f6 to 969685a368ceabef622c888ace30f2131a666bb1
comment:10 Changed 6 years ago by
This should be good now, but patchbots will not have a look. Please check and review.
comment:11 Changed 6 years ago by
Failing doctest from most recent alterations:
File "src/sage/repl/rich_output/output_graphics3d.py", line 159, in sage.repl.rich_output.output_graphics3d.OutputSceneCanvas3d.example Failed example: rich_output.canvas3d Expected: buffer containing 649 bytes Got: buffer containing 829 bytes
comment:12 Changed 5 years ago by
 Commit changed from 969685a368ceabef622c888ace30f2131a666bb1 to fbd8b10344636dc09211356be7daaf2f1ff25f38
comment:13 Changed 5 years ago by
done, should be good now.
comment:14 Changed 5 years ago by
 Status changed from needs_review to positive_review
comment:15 Changed 5 years ago by
 Branch changed from u/chapoton/22253 to fbd8b10344636dc09211356be7daaf2f1ff25f38
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
trac 22253 fixing json repr of index_face_set