zooniverse / front-end-monorepo

A rebuild of the front-end for zooniverse.org
https://www.zooniverse.org
Apache License 2.0
104 stars 30 forks source link

Classifier: SVGPoint deprecated, replacement needed #2268

Closed shaunanoordin closed 2 years ago

shaunanoordin commented 3 years ago

Code Update (Refactor?) Required

Package: lib-classifier Follows #2255

Context: while refactoring how the classifier passes svg references around with the SVGContext, @eatyourgreens noticed that a necessary component of our svg-related calculations is now out of date and in danger of no longer being supported.

Overview:

Further Reading:

Dev Notes

SVGPoints are used in two locations in lib-classifier:

In InteractionLayer.js for example, we have this code:

function getEventOffset(event) {
  const svgPoint = svg.createSVGPoint()
  svgPoint.x = event.clientX
  svgPoint.y = event.clientY
  const svgEventOffset = svgPoint.matrixTransform(getScreenCTM().inverse())
  return svgEventOffset
}

// For context, getScreenCTM() belongs to the main transform layer, aka the <g> element that contains the transform ops performed on the subject image + its annotations (the InteractionLayer).

SVGPoint's .matrixTransform() is basically the lynchpin that automagically transmogrifies event coordinates/DOM coordinates to SVG-relative coordinates.

If all else fails, I think it's possible to reverse-engineer the matrix transformation calculations. mainTransformLayer.getScreenCTM() still works and returns a DOMMatrix with all the necessary values that someone smarter than me can mathemagically figure out.

Surely this is solvable. I mean... it's just a matrix, right?

The Matrix

Notes to self:

Status

Solution requires research.

At the moment, our Classifier continues to work, and there are no indications that modern browsers are going to pull out SVGPoint support any time soon, but that "deprecation alarm" is unsettling enough as is.

shaunanoordin commented 3 years ago

Copy-pasting some new comments I made in 2255:

...right now the Classifier is passing around <svg> and <g id="transformation_layer">.getScreenCTM() via SVGContext. (See: SingleImageViewer.js)

That might be a bit confusing because it's passing the reference to one element, and the function to a DIFFERENT element.

It might make a bit more sense for the SVGContext to instead pass around <svg> and <g id="transformation_layer"> instead (i.e. two references to different elements) for consistency; this would make it super clear to the context-consuming components (i.e. draggable.js, InteractionLayer.js) which element it's pulling getScreenCTM() from.

eatyourgreens commented 3 years ago
const svgPoint = new DOMPoint(x,y)
const eventOffset = svgPoint(getScreenCTM(). Inverse())

might be worth trying.

shaunanoordin commented 3 years ago

(Accidentally posted this in https://github.com/zooniverse/front-end-monorepo/issues/2273#issuecomment-869997664 instead of this Issue, whoops.)

DOMPoint might be IS a valid replacement for SVGPoint! This class also has the matrixTransform() function... I think... it's just really poorly documented.

shaunanoordin commented 2 years ago

Resolved by #2277