fix: annotation behavioral fixes and IO stability #175
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -76,6 +76,7 @@ OSDAnnotations.Convertor.register("geo-json", class extends OSDAnnotations.Conve | |||||
| return res; | ||||||
| }, | ||||||
| "polyline": (object) => this._asGEOJsonFeature(object, "LineString", ["points"], false), | ||||||
| "line": (object) => this._asGEOJsonFeature(object, "LineString", [], false), | ||||||
| "point": (object) => { | ||||||
| object = this._asGEOJsonFeature(object, "Point"); | ||||||
| object.geometry.coordinates = object.geometry.coordinates[0] || []; | ||||||
|
|
@@ -89,12 +90,15 @@ OSDAnnotations.Convertor.register("geo-json", class extends OSDAnnotations.Conve | |||||
| "ruler": (object) => { | ||||||
| const factory = this.context.getAnnotationObjectFactory(object.factoryID); | ||||||
| const converter = OSDAnnotations.AnnotationObjectFactory.withArrayPoint; | ||||||
| object._objects = object.objects; // todo ugly, factory works with live objects | ||||||
| const props = factory.copyNecessaryProperties(object, [], true); | ||||||
|
Contributor
There was a problem hiding this comment. If
Suggested change
|
||||||
| props.text = object._objects?.[1]?.text; | ||||||
| return { | ||||||
| geometry: { | ||||||
| type: "LineString", | ||||||
| coordinates: factory?.toPointArray(object, converter, fabric.Object.NUM_FRACTION_DIGITS) | ||||||
| }, | ||||||
| properties: props | ||||||
| }; | ||||||
| }, | ||||||
| }; | ||||||
|
|
@@ -110,6 +114,12 @@ OSDAnnotations.Convertor.register("geo-json", class extends OSDAnnotations.Conve | |||||
| (object, geometry) => object.points = geometry.map(ring => this._toNativeRing(ring))), | ||||||
| "polyline": (object) => this._getAsNativeObject(object, | ||||||
| (object, geometry) => object.points = this._toNativeRing(geometry, false)), | ||||||
| "line": (object) => { | ||||||
| const factory = this.context.getAnnotationObjectFactory("line"); | ||||||
| const params = factory.fromPointArray(object.geometry.coordinates, ([x, y]) => ({ x, y })); | ||||||
| const opts = $.extend({}, this.context.presets.getCommonProperties(), object.properties); | ||||||
| return factory.create(params, opts); | ||||||
|
Comment on lines
+118
to
+121
Contributor
There was a problem hiding this comment. If the const factory = this.context.getAnnotationObjectFactory("line");
if (!factory) return null;
const params = factory.fromPointArray(object.geometry.coordinates, ([x, y]) => ({ x, y }));
const opts = $.extend({}, this.context.presets.getCommonProperties(), object.properties);
return factory.create(params, opts); |
||||||
| }, | ||||||
| "point": (object) => this._getAsNativeObject(object, (object, geometry) => { | ||||||
| //todo not necessary? left/top are already probably present in props | ||||||
| object.left = geometry[0]; | ||||||
|
|
@@ -122,7 +132,16 @@ OSDAnnotations.Convertor.register("geo-json", class extends OSDAnnotations.Conve | |||||
| object.top = geometry[1]; | ||||||
| return object; | ||||||
| }), | ||||||
| "ruler": (object) => { | ||||||
| const factory = this.context.getAnnotationObjectFactory("ruler"); | ||||||
| const params = factory.fromPointArray(object.geometry.coordinates, ([x, y]) => ({ x, y })); | ||||||
| const opts = $.extend({}, this.context.presets.getCommonProperties(), object.properties); | ||||||
| const obj = factory.create(params, opts); | ||||||
|
Comment on lines
+136
to
+139
Contributor
There was a problem hiding this comment. If the const factory = this.context.getAnnotationObjectFactory("ruler");
if (!factory) return null;
const params = factory.fromPointArray(object.geometry.coordinates, ([x, y]) => ({ x, y }));
const opts = $.extend({}, this.context.presets.getCommonProperties(), object.properties);
const obj = factory.create(params, opts); |
||||||
| if (object.properties?.text && obj?._objects?.[1]) { | ||||||
| obj._objects[1].set({ text: object.properties.text }); | ||||||
| } | ||||||
| return obj; | ||||||
| }, | ||||||
| }; | ||||||
|
|
||||||
| // todo support unpacking like qupath does | ||||||
|
|
||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -135,6 +135,7 @@ OSDAnnotations.Convertor.register("qupath", class extends OSDAnnotations.Convert | |
| return res; | ||
| }, | ||
| "polyline": (object, preset) => this._asGEOJsonFeature(object, preset, "LineString", ["points"], false), | ||
| "line": (object, preset) => this._asGEOJsonFeature(object, preset, "LineString", [], false), | ||
| "point": (object, preset) => { | ||
| object = this._asGEOJsonFeature(object, preset, "Point"); | ||
| object.geometry.coordinates = object.geometry.coordinates[0] || []; | ||
|
|
@@ -145,7 +146,10 @@ OSDAnnotations.Convertor.register("qupath", class extends OSDAnnotations.Convert | |
| object.geometry.coordinates = object.geometry.coordinates[0] || []; | ||
| return object; | ||
| }, | ||
| "ruler": (object, preset) => { | ||
| object._objects = object.objects; // todo ugly, factory used underneath expects live group with _objects | ||
| return this._asGEOJsonFeature(object, preset, "LineString"); | ||
| }, | ||
| }; | ||
|
|
||
| _decodeMulti(object, featureParentDict, type) { | ||
|
|
@@ -229,6 +233,12 @@ OSDAnnotations.Convertor.register("qupath", class extends OSDAnnotations.Convert | |
| let encoded = this.encoders[obj.factoryID]?.(obj, presets.find(p => p.presetID == obj.presetID)); | ||
| if (encoded) { | ||
| encoded.type = "Feature"; | ||
| encoded.properties = encoded.properties || {}; | ||
| encoded.properties.xopatFactoryID = obj.factoryID; | ||
| if (obj.factoryID === "ruler") { | ||
| const txt = obj._objects?.[1]?.text; | ||
| if (txt) encoded.properties.xopatRulerText = txt; | ||
| } | ||
| if (this.options.serialize) encoded = JSON.stringify(encoded); | ||
| result.objects.push(encoded); | ||
| } | ||
|
|
@@ -299,7 +309,25 @@ OSDAnnotations.Convertor.register("qupath", class extends OSDAnnotations.Convert | |
| throw "Invalid feature! "; | ||
| } | ||
|
|
||
| let result; | ||
| const xid = object.properties?.xopatFactoryID; | ||
| if (xid === "line" || xid === "ruler") { | ||
| const factory = this.context.getAnnotationObjectFactory(xid); | ||
| if (factory) { | ||
| const off = this.offset; | ||
| const deconv = off | ||
| ? ([x, y]) => ({ x: x + off.x, y: y + off.y }) | ||
| : ([x, y]) => ({ x, y }); | ||
| const params = factory.fromPointArray(object.geometry.coordinates, deconv); | ||
| result = factory.create(params, this.context.presets.getCommonProperties()); | ||
| if (xid === "ruler" && object.properties?.xopatRulerText && result?._objects?.[1]) { | ||
| result._objects[1].set({ text: object.properties.xopatRulerText }); | ||
| } | ||
| } | ||
| } | ||
| if (!result) { | ||
| result = this.decoders[object.geometry.type]?.(object.geometry, object); | ||
| } | ||
|
|
||
| if (Array.isArray(result)) { | ||
| //MultiPolygon, MultiPoint, etc. | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,6 +42,12 @@ OSDAnnotations.Ruler = class extends OSDAnnotations.AnnotationObjectFactory { | |
| */ | ||
| configure(instance, options) { | ||
| if (instance.type === "group") { | ||
| // Legacy/native imports may lack color on the group; recover from preset | ||
| // so the inner line gets a stroke and renders. | ||
| if (!options.color && instance.presetID) { | ||
| const preset = this._presets?.get?.(instance.presetID); | ||
| if (preset?.color) options = $.extend({}, options, { color: preset.color }); | ||
| } | ||
| this._configureParts(instance.item(0), instance.item(1), options); | ||
| this._configureWrapper(instance, instance.item(0), instance.item(1), options); | ||
| } | ||
|
|
@@ -287,6 +293,9 @@ OSDAnnotations.Ruler = class extends OSDAnnotations.AnnotationObjectFactory { | |
| } | ||
|
|
||
| const props = this._presets.getCommonProperties(); | ||
| // Helper line was created from the active preset and carries its color; | ||
| // forward it so the group is serialized with color and the inner line keeps a stroke after import. | ||
| if (line.color) props.color = line.color; | ||
| obj = this._createWrap(obj, props); | ||
| obj.presetID = pid; | ||
| this._context.addAnnotation(obj); | ||
|
|
@@ -331,18 +340,27 @@ OSDAnnotations.Ruler = class extends OSDAnnotations.AnnotationObjectFactory { | |
| } | ||
|
|
||
| toPointArray(obj, converter, digits=undefined, quality=1) { | ||
| // Accept both live group (obj._objects) and serialized group (obj.objects). | ||
| const inner = obj._objects || obj.objects; | ||
| const line = inner ? inner[0] : null; | ||
| if (!line) return []; | ||
|
|
||
| // Inner line's x1..y2 are centered around the group's bbox center; | ||
| // compose with the group's left/top + half-extents to get absolute canvas coords. | ||
| const width = Number.isFinite(obj.width) ? obj.width : 0; | ||
| const height = Number.isFinite(obj.height) ? obj.height : 0; | ||
| const cx = (obj.left || 0) + width / 2; | ||
| const cy = (obj.top || 0) + height / 2; | ||
| let x1 = cx + (line.x1 || 0); | ||
| let y1 = cy + (line.y1 || 0); | ||
| let x2 = cx + (line.x2 || 0); | ||
| let y2 = cy + (line.y2 || 0); | ||
|
|
||
| if (digits !== undefined) { | ||
| x1 = parseFloat(Number(x1).toFixed(digits)); | ||
| y1 = parseFloat(Number(y1).toFixed(digits)); | ||
| x2 = parseFloat(Number(x2).toFixed(digits)); | ||
| y2 = parseFloat(Number(y2).toFixed(digits)); | ||
| } | ||
| return [converter(x1, y1), converter(x2, y2)]; | ||
| } | ||
|
|
@@ -360,6 +378,15 @@ OSDAnnotations.Ruler = class extends OSDAnnotations.AnnotationObjectFactory { | |
| _configureLine(line, options) { | ||
| const lineOptions = Object.assign({}, options); | ||
| lineOptions.stroke = options.color; | ||
| // Don't carry the wrap group's positional/sizing props onto the inner line: | ||
| // the inner line's coords are local to the group and managed independently; | ||
| // applying the group's bbox here moves the line off-canvas (invisible). | ||
| delete lineOptions.left; | ||
| delete lineOptions.top; | ||
| delete lineOptions.width; | ||
| delete lineOptions.height; | ||
| delete lineOptions.scaleX; | ||
| delete lineOptions.scaleY; | ||
|
|
||
| $.extend(line, { | ||
| scaleX: 1, | ||
|
|
@@ -414,8 +441,17 @@ OSDAnnotations.Ruler = class extends OSDAnnotations.AnnotationObjectFactory { | |
| } | ||
|
|
||
| _createWrap(parts, options) { | ||
| const line = parts[0]; | ||
| const wrap = new fabric.Group(parts, { | ||
| originX: 'left', | ||
| originY: 'top', | ||
| left: line.left, | ||
| top: line.top, | ||
| width: line.width, | ||
| height: line.height, | ||
| }); | ||
| this._configureWrapper(wrap, wrap.item(0), wrap.item(1), options); | ||
| wrap.setCoords(); | ||
| return wrap; | ||
| } | ||
| }; | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing the entire fabric object
objas theoptionsparameter tofactory.configureis highly inefficient and dangerous. InRuler.configure,$.extend({}, options, ...)andObject.assign({}, options)are called, which will attempt to clone/copy all properties of the fabric instance (including circular references, canvas references, etc.). This can lead to severe performance degradation, memory bloat, or runtime errors.Instead, extract and pass only the necessary properties as a plain object.