Introducing 3D annotations #1638

etpinard · 2017-04-28T21:28:43Z

resolves #751 and more precisely implements #751 (comment).

This PR is still a very much WIP. Here are some TODOs:

get the autorange logic hooked in
get this to work with category axes types
register the new logic under components/annotations/ (or a new components/annotations3d maybe?)
DRY up defaults
tests

- one `data-index` attr on the top-level ann group should be enough.

- instead of looking through the whole graph div

- so that they can be mutated downstream

- which expects an option object, xaxis and yaxis - generalize className given to annotation to group and base attribute string (for editable: true) to accommodate 3d annotations - use `_input` ref instead of `gd.layout` to grab user layout options

- add attrs and defaults to scene containers - N.B. x/y/z are always in data coords ax/ay are always in px coords - mock xaxis and yaxis on scene updates so that their l2p method updates the projection data coord in the x-y plane - and annotations handler in render loop

- as this problem is ill-defined, mouse xy -> scene xyz as infinity-many solutions. - N.B. editing arrow tail position (i.e. ax, ay) and annotation text works!

This reverts commit 1a8f1ed.

etpinard · 2017-05-24T14:34:39Z

src/components/annotations/draw.js

@@ -168,7 +168,6 @@ function drawOne(gd, index) {
    var font = options.font;

    var annText = annTextGroupInner.append('text')
-        .classed('annotation', true)


This commit is reverted in ababc49 to make the tests added in #1683 ✅

I think you've hit on a flaw in how github displays comments... I'm reading:

This commit is reverted in ababc49 to make the tests added in #1683 ✅

but I'm looking at a31549d which came after ababc49 and rereverts the change originally made in 1a8f1ed 💫

Anyway, totally approve of this change. My best guess is these got the same .annotation class because at some point in the distant past they weren't all encapsulated in one group like they are now, and I wanted to be able to more easily select the whole annotation. Has always been confusing to deal with...

In brief, as of a31549d, the annotations text nodes have class name annotation-text.

- coerce (x, y, z) in scene.plot after the ax._categories is filled - use .r2l to update annotation position - could be cleaned up (by e.g. wrapping the 3d annotation default into a required subroutine).

- N.B. does not take into consideration the arrowhead (debatable)

etpinard · 2017-05-24T23:24:13Z

Ok. All the functionality is in (I think), so I'll tagged this as reviewable.

I'm currently thinking about putting this new 3d annotation code under a new component components/annotations3d/ which would reuse much of components/annotations/ but would remain separate allowing users that do not need 3d annotations (or 3d graphs 😏 ) for that matter to trim down their bundle size even more in v2 (i.e. when components will be taken out of the core module).

Any objections before I shuffle the new code around?

- so that the `selectAll('.annotation')` call Annotation.draw doesn't clear 3d annotation text nodes.

alexcjohnson · 2017-05-25T19:11:21Z

src/plots/gl3d/scene.js

+
+    ann._xa = {};
+    Lib.extendFlat(ann._xa, base);
+    Axes.setConvert(ann._xa);


what do you need from setConvert, if you're overriding _offset and l2p?

what do you need from setConvert, if you're overriding _offset and l2p?

Oh I see - ax.l2p is now controlling all the other 2p conversions (instead of the local var l2p) so you get those for free. Seems fine - tiny tiny 🐎 hit from having to look up attributes more often... but should be negligible.

Oh I see - ax.l2p is now controlling all the other 2p conversions (instead of the local var l2p) so you get those for free

Exactly.

alexcjohnson · 2017-05-25T19:12:48Z

Any objections before I shuffle the new code around?

sounds reasonable, go for it!

alexcjohnson · 2017-05-25T19:31:17Z

src/components/annotations/draw.js

+    var fullLayout = gd._fullLayout;
+    var gs = gd._fullLayout._size;
+
+    var className = options._scene ?


elsewhere _scene is an object, but this one is an id - can we call it _sceneId instead?

Good call. Done in c084da0

alexcjohnson · 2017-05-25T19:45:25Z

src/plots/gl3d/layout/layout_attributes.js

+            ].join(' ')
+        },
+        ax: {
+            valType: 'any',


'number' now since it's always going to be in px?

Thanks! Done in adfc305

alexcjohnson · 2017-05-25T19:57:01Z

src/components/annotations/draw.js

        .attr('data-index', String(index))
        .style('opacity', options.opacity);

    // another group for text+background so that they can rotate together
    var annTextGroup = annGroup.append('g')
-        .classed('annotation-text-g', true)
-        .attr('data-index', String(index));


👍 another relic 🔪

- N.B. leave (small) autorange block in scene.js

- which coerces all non-position attributes for annotations and annotations3d - automatically adds supports for 'hovertext' and 'hoverlabel' for annotation3d 🎉

- as they can only be set in pixels.

etpinard · 2017-05-25T20:38:34Z

src/components/annotations3d/attributes.js

@@ -75,13 +75,13 @@ module.exports = {
    arrowwidth: annAtts.arrowwidth,
    standoff: annAtts.standoff,
    hovertext: annAtts.hovertext,
-    hoverlabel: annAtts.hoverlabel
+    hoverlabel: annAtts.hoverlabel,
+    captureevents: annAtts.captureevents


With this, hover labels on 3d annotations are working:

off:

Plotly.newPlot('graph', [{ type: 'scatter3d', x: [1, 2, 3], y: [1, 2, 3], z: [1, 2, 1] }], { scene: { annotations: [{ text: 'hello', x: 2, y: 2, z: 2, ay: -100, hovertext: 'HELLO', hoverlabel: { bgcolor: 'red', font: {size: 20} } }] }, })

what about the plotly_clickannotation event? Looks like that might need a sceneId if there is one? Which brings up the point that for extensibility to other non-cartesian subplots perhaps it should really be called subplotId...

commit 8b8a088 generalises drawRaw

commit 1d5606a add subplotId to the plotly_clickannotation event data + 🔒 it down.

After 1d5606a, this and that assertions are failing, because the event data now contains subplotId: false for cartesian/paper-ref annotations.

@alexcjohnson would you prefer leaving subplotId out of the cartesian/paper-ref annotation event data or keep it as subplotId: false?

would you prefer leaving subplotId out of the cartesian/paper-ref annotation event data or keep it as subplotId: false

haha didn't see this before my comment

yeah, lets omit it.

done in 46ab63d

- instead of expecting it on options._subplotId

- add hover/click tests for 3d annotations

alexcjohnson · 2017-05-26T16:13:25Z

That's all from my side, looks great! 💃

etpinard added 8 commits April 28, 2017 17:08

replace forEach with for-loop over ['x', 'y']

aa58118

don't (uselessly) add data-index to ann text+bg & arrow groups

8b8bdea

- one `data-index` attr on the top-level ann group should be enough.

don't (uselessly) add annotation class to ann text group inner

1a8f1ed

fine tune annotation arrow selector

f4b14e6

- instead of looking through the whole graph div

use ax.?2? while composing convert methods

4dc16e4

- so that they can be mutated downstream

split annotation drawOne into drawRaw

8e6ab7d

- which expects an option object, xaxis and yaxis - generalize className given to annotation to group and base attribute string (for editable: true) to accommodate 3d annotations - use `_input` ref instead of `gd.layout` to grab user layout options

don't try to edit x,y,z positions for 3D annotations

b7ab656

- as this problem is ill-defined, mouse xy -> scene xyz as infinity-many solutions. - N.B. editing arrow tail position (i.e. ax, ay) and annotation text works!

etpinard added status: in progress feature something new labels Apr 28, 2017

etpinard added 3 commits May 9, 2017 10:40

Merge branch 'master' into gl3d-annotations

fbdcee8

Merge branch 'master' into gl3d-annotations

981b1a1

rename anchor -> anchor3 so that it doesn't conflict w/ for-loop block

94b940d

etpinard added this to the 1.28.0 milestone May 18, 2017

etpinard mentioned this pull request May 19, 2017

No shape and no marker 'cross-thin' on scattergeo plot #1707

Closed

etpinard added 2 commits May 24, 2017 09:47

Merge branch 'master' into gl3d-annotations

8ac2551

Revert "don't (uselessly) add annotation class to ann text group inner"

ababc49

This reverts commit 1a8f1ed.

etpinard commented May 24, 2017

View reviewed changes

etpinard added 5 commits May 24, 2017 19:19

fix 3d annotations on type: 'category' axes

7cfba36

- coerce (x, y, z) in scene.plot after the ax._categories is filled - use .r2l to update annotation position - could be cleaned up (by e.g. wrapping the 3d annotation default into a required subroutine).

make sure to clear 3d annotations when parent scene gets removed

8f9e370

implement autorange logic for annotations

1bbabad

- N.B. does not take into consideration the arrowhead (debatable)

first pass 3d annotations jasmine tests

9520943

first pass 3d annotation image test support

f61cd4d

etpinard added status: reviewable and removed status: in progress labels May 24, 2017

etpinard added 2 commits May 25, 2017 10:48

try to make test that update the scene camera more robust

8cb10c1

replace 'annotation' class for 'annotation-text' for annation text nodes

a31549d

- so that the `selectAll('.annotation')` call Annotation.draw doesn't clear 3d annotation text nodes.

alexcjohnson reviewed May 25, 2017

View reviewed changes

etpinard added 4 commits May 25, 2017 16:01

mv 3d annotations code out of plots/gl3d into component/annotations3d

bd9c867

- N.B. leave (small) autorange block in scene.js

add common annotations defaults modules

21c8cc6

- which coerces all non-position attributes for annotations and annotations3d - automatically adds supports for 'hovertext' and 'hoverlabel' for annotation3d 🎉

declare ax and ay as valType: number for annotation3d

adfc305

- as they can only be set in pixels.

rename _scene ref to scene id in annotation container _sceneId

c084da0

etpinard force-pushed the gl3d-annotations branch from 21a5a0e to 876b37b Compare May 25, 2017 20:26

put 'captureevents' under common annotation defaults

ef985cf

etpinard force-pushed the gl3d-annotations branch from 876b37b to ef985cf Compare May 25, 2017 20:30

etpinard commented May 25, 2017

View reviewed changes

etpinard added 4 commits May 25, 2017 17:28

Merge branch 'master' into gl3d-annotations

0b4214f

pass subplotId as arg to Annotations.drawRaw

8b8a088

- instead of expecting it on options._subplotId

include subplotId in clickannotation event data

1d5606a

- add hover/click tests for 3d annotations

rm subplotId key from clickannotations event data when falsy

46ab63d

etpinard merged commit 6b7c9d7 into master May 26, 2017

etpinard deleted the gl3d-annotations branch May 26, 2017 16:14

etpinard mentioned this pull request Jun 14, 2017

hide off-plot annotations #1786

Merged

etpinard mentioned this pull request Feb 20, 2020

anchor shapes to map axes #4594

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introducing 3D annotations #1638

Introducing 3D annotations #1638

etpinard commented Apr 28, 2017 •

edited

Loading

etpinard May 24, 2017

alexcjohnson May 25, 2017

etpinard May 25, 2017 •

edited

Loading

etpinard commented May 24, 2017 •

edited

Loading

alexcjohnson May 25, 2017

alexcjohnson May 25, 2017

etpinard May 25, 2017

alexcjohnson commented May 25, 2017

alexcjohnson May 25, 2017

etpinard May 25, 2017

alexcjohnson May 25, 2017

etpinard May 25, 2017

alexcjohnson May 25, 2017

etpinard May 25, 2017 •

edited

Loading

alexcjohnson May 25, 2017

etpinard May 26, 2017 •

edited

Loading

etpinard May 26, 2017

alexcjohnson May 26, 2017

etpinard May 26, 2017

alexcjohnson commented May 26, 2017

Introducing 3D annotations #1638

Introducing 3D annotations #1638

Conversation

etpinard commented Apr 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard May 25, 2017 • edited Loading

Choose a reason for hiding this comment

etpinard commented May 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcjohnson commented May 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard May 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard May 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcjohnson commented May 26, 2017

etpinard commented Apr 28, 2017 •

edited

Loading

etpinard May 25, 2017 •

edited

Loading

etpinard commented May 24, 2017 •

edited

Loading

etpinard May 25, 2017 •

edited

Loading

etpinard May 26, 2017 •

edited

Loading