From d45b7162aaf12dface33fcfa5fdcacf4962edd50 Mon Sep 17 00:00:00 2001 From: Aiosa Date: Wed, 27 May 2026 13:58:50 +0200 Subject: [PATCH] fix: annotation behavioral fixes and IO stability --- modules/annotations/annotations.js | 2 + modules/annotations/convert/asapXml.js | 101 +++++++++++++++--- modules/annotations/convert/geoJSON.js | 23 +++- modules/annotations/convert/qupath.js | 32 +++++- .../annotations/objectAdvancedFactories.js | 58 ++++++++-- modules/annotations/objectGenericFactories.js | 39 +++++-- plugins/annotations/annotationsGUI.js | 2 +- 7 files changed, 216 insertions(+), 41 deletions(-) diff --git a/modules/annotations/annotations.js b/modules/annotations/annotations.js index cd74568f..c7e60434 100644 --- a/modules/annotations/annotations.js +++ b/modules/annotations/annotations.js @@ -2200,6 +2200,8 @@ in order to work. Did you maybe named the ${type} factory implementation differe } self.checkLayer(obj); self.checkAnnotation(obj); + const factory = self.getAnnotationObjectFactory(obj.factoryID); + factory?.configure?.(obj, obj); obj.on('selected', self._objectClicked.bind(self)); obj.on('deselected', self._objectDeselected.bind(self)); _this.insertAt(obj, insertion++); diff --git a/modules/annotations/convert/asapXml.js b/modules/annotations/convert/asapXml.js index 08069bcb..3b180c71 100644 --- a/modules/annotations/convert/asapXml.js +++ b/modules/annotations/convert/asapXml.js @@ -1,4 +1,19 @@ //ASAP XML not yet fully tested, does not render well all objects, problem with hierarchies +const ASAP_TYPE_BY_FACTORY = { + rect: "Rectangle", + polygon: "Polygon", + multipolygon: "Polygon", + ellipse: "Polygon", + polyline: "Spline", + line: "Spline", + ruler: "Spline", + point: "Dot", + text: "Dot", +}; +const ASAP_LOSSY_FACTORIES = new Set([ + "multipolygon", "ellipse", "polyline", "line", "ruler", "text" +]); + OSDAnnotations.Convertor.register("asap-xml", class extends OSDAnnotations.Convertor.IConvertor { static title = 'ASAP-XML Annotations'; static description = 'ASAP-compatible XML Annotations Format'; @@ -52,6 +67,7 @@ OSDAnnotations.Convertor.register("asap-xml", class extends OSDAnnotations.Conve const result = {}; let wasError = ''; + const lossyTypes = new Set(); let doc = document.implementation.createDocument("", "", null); const presetsIdSet = new Set(); @@ -70,9 +86,7 @@ OSDAnnotations.Convertor.register("asap-xml", class extends OSDAnnotations.Conve let factory = this.context.getAnnotationObjectFactory(obj.factoryID); if (factory) { - // Todo some better reporting mechanism if (factory.factoryID === "multipolygon") { - wasError = 'ASAP XML Does not support multipolygons - saved as polygon.'; coordinates = this.context.polygonFactory.toPointArray({points: obj.points[0]}, OSDAnnotations.AnnotationObjectFactory.withArrayPoint); } else { @@ -84,7 +98,21 @@ OSDAnnotations.Convertor.register("asap-xml", class extends OSDAnnotations.Conve continue; } } - xml_annotation.setAttribute("Type", "Polygon"); + const asapType = ASAP_TYPE_BY_FACTORY[factory?.factoryID] || "Polygon"; + xml_annotation.setAttribute("Type", asapType); + // Per-annotation factoryID (custom attribute, ASAP ignores unknown attrs). + // Required because a preset's FactoryID is a default, but its annotations may differ. + if (factory?.factoryID) { + xml_annotation.setAttribute("xopatFactoryID", factory.factoryID); + } + if (factory && ASAP_LOSSY_FACTORIES.has(factory.factoryID)) { + lossyTypes.add(factory.factoryID); + } + if (factory?.factoryID === "ruler") { + const inner = obj._objects || obj.objects; + const txt = inner?.[1]?.text; + if (txt) xml_annotation.setAttribute("Description", txt); + } //todo attr name could be set from preset xml_annotation.setAttribute("Name", "Annotation " + i); @@ -150,9 +178,14 @@ OSDAnnotations.Convertor.register("asap-xml", class extends OSDAnnotations.Conve //todo check for consitency presetsIdSet? } + if (lossyTypes.size) { + const lossyMsg = `ASAP-XML is lossy for: ${[...lossyTypes].join(", ")}. ` + + `Re-import in xopat preserves class via preset FactoryID; other tools see only the geometry.`; + wasError = wasError ? wasError + "\n" + lossyMsg : lossyMsg; + } // Todo - create some unified checking mechanism that reports on export issues if (wasError) { - Dialogs.show(wasError, 15000, Dialogs.MSG_ERR); + Dialogs.show(wasError, 15000, Dialogs.MSG_WARN); } return result; @@ -210,22 +243,56 @@ OSDAnnotations.Convertor.register("asap-xml", class extends OSDAnnotations.Conve for (const coordElem of coords.getElementsByTagName("Coordinate")) { const index = Number.parseInt(coordElem.getAttribute("Order")); pointArray[index] = { - x: Number.parseInt(coordElem.getAttribute("X")), - y: Number.parseInt(coordElem.getAttribute("Y")) + x: Number.parseFloat(coordElem.getAttribute("X")), + y: Number.parseFloat(coordElem.getAttribute("Y")) } } - const presetID = elem.getAttribute("PartOfGroup"); - - //todo support: Dot, Rectangle, Polygon, Spline, and PointSet by implementation of general annotation structure - //todo attr name could be set as category custom meta - annotations.push({ - type: "polygon", - points: pointArray, - presetID: presetID, - factoryID: "polygon", - color: elem.getAttribute("Color") || undefined - }); + const rawPresetID = elem.getAttribute("PartOfGroup"); + const presetID = Number.parseInt(rawPresetID) || rawPresetID; + const preset = presets[presetID]; + // Prefer per-annotation factoryID (custom attribute) over the preset's default factoryID: + // a preset's FactoryID is just the default; its annotations may be of other types. + const xopatFacID = elem.getAttribute("xopatFactoryID"); + let facID = xopatFacID || preset?.factoryID || "polygon"; + // ASAP-XML drops multipolygon rings on export (only outer ring of first polygon is kept); + // decode that single ring as a polygon to avoid Multipolygon.fromPointArray misinterpreting flat points. + if (facID === "multipolygon") facID = "polygon"; + const factory = this.context.getAnnotationObjectFactory(facID); + const color = elem.getAttribute("Color") || preset?.color || undefined; + + let pushed = false; + if (factory && typeof factory.fromPointArray === "function" && + typeof factory.create === "function") { + try { + const arrPts = pointArray.map(p => [p.x, p.y]); + const params = factory.fromPointArray(arrPts, ([x, y]) => ({ x, y })); + const opts = { + color, + presetID, + factoryID: facID, + ...(this.context.presets.getCommonProperties?.() || {}), + }; + const obj = factory.create(params, opts); + if (facID === "ruler") { + const desc = elem.getAttribute("Description"); + if (desc && obj?._objects?.[1]) obj._objects[1].set({ text: desc }); + } + annotations.push(obj); + pushed = true; + } catch (e) { + console.warn("ASAP-XML decode: factory reconstruction failed, falling back to polygon", facID, e); + } + } + if (!pushed) { + annotations.push({ + type: "polygon", + points: pointArray, + presetID, + factoryID: "polygon", + color + }); + } } return { diff --git a/modules/annotations/convert/geoJSON.js b/modules/annotations/convert/geoJSON.js index 45e80b91..8b7a532e 100644 --- a/modules/annotations/convert/geoJSON.js +++ b/modules/annotations/convert/geoJSON.js @@ -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); + props.text = object._objects?.[1]?.text; return { geometry: { type: "LineString", coordinates: factory?.toPointArray(object, converter, fabric.Object.NUM_FRACTION_DIGITS) }, - properties: factory.copyNecessaryProperties(object, [], true) + 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); + }, "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) => this._getAsNativeObject(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); + if (object.properties?.text && obj?._objects?.[1]) { + obj._objects[1].set({ text: object.properties.text }); + } + return obj; + }, }; // todo support unpacking like qupath does diff --git a/modules/annotations/convert/qupath.js b/modules/annotations/convert/qupath.js index 15c57a1b..2826f93b 100644 --- a/modules/annotations/convert/qupath.js +++ b/modules/annotations/convert/qupath.js @@ -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) => this._asGEOJsonFeature(object, preset, "LineString"), + "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 = this.decoders[object.geometry.type]?.(object.geometry, object); + 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. diff --git a/modules/annotations/objectAdvancedFactories.js b/modules/annotations/objectAdvancedFactories.js index acd7e5f3..4b148dc6 100644 --- a/modules/annotations/objectAdvancedFactories.js +++ b/modules/annotations/objectAdvancedFactories.js @@ -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) { - const line = obj._objects ? obj._objects[0] : []; - - let x1 = line.x1; - let y1 = line.y1; - let x2 = line.x2; - let y2 = line.y2; + // 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(x1.toFixed(digits)); - y1 = parseFloat(y1.toFixed(digits)); - x2 = parseFloat(x2.toFixed(digits)); - y2 = parseFloat(y2.toFixed(digits)); + 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 wrap = new fabric.Group(parts); + 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; } }; diff --git a/modules/annotations/objectGenericFactories.js b/modules/annotations/objectGenericFactories.js index 243216d8..e8a6c49d 100644 --- a/modules/annotations/objectGenericFactories.js +++ b/modules/annotations/objectGenericFactories.js @@ -724,6 +724,11 @@ OSDAnnotations.Point = class extends OSDAnnotations.Ellipse { return `Point [${Math.round(ofObject.top)}, ${Math.round(ofObject.left)}]`; } + selected(theObject) { + this.renderAllControls(theObject); + theObject.setControlsVisibility({ private: true }); + } + /** * todo necessary? we use ellipse * @param {object} parameters @@ -1285,6 +1290,11 @@ OSDAnnotations.Line = class extends OSDAnnotations.AnnotationObjectFactory { return this._current; } + selected(theObject) { + this.renderAllControls(theObject); + theObject.setControlsVisibility({ private: true }); + } + /** * @param {Array} parameters array of objects with {x, y} properties (points) * @param {Object} options see parent class @@ -1314,6 +1324,12 @@ OSDAnnotations.Line = class extends OSDAnnotations.AnnotationObjectFactory { return ["x1", "x2", "y1", "y2"]; } + exports() { + // Force left/top into serialized output; fabric.Line strips them as defaults + // when they equal 0 (common with center-origin lines), losing the position on re-import. + return ["left", "top"]; + } + updateRendering(ofObject, preset, visualProperties, defaultVisualProperties) { visualProperties.modeOutline = true; super.updateRendering(ofObject, preset, visualProperties, defaultVisualProperties); @@ -1520,16 +1536,23 @@ OSDAnnotations.Line = class extends OSDAnnotations.AnnotationObjectFactory { * @return {Array} array of items returned by the converter - points */ toPointArray(obj, converter, digits=undefined, quality=1) { + // fabric.Line stores x1..y2 centered around the bbox center (calcLinePoints output); + // compose with left/top + width/2/height/2 to get absolute canvas coords. + const width = Number.isFinite(obj.width) ? obj.width : Math.abs((obj.x2 || 0) - (obj.x1 || 0)); + const height = Number.isFinite(obj.height) ? obj.height : Math.abs((obj.y2 || 0) - (obj.y1 || 0)); + const cx = (obj.left || 0) + width / 2; + const cy = (obj.top || 0) + height / 2; + let x1 = cx + (obj.x1 || 0); + let y1 = cy + (obj.y1 || 0); + let x2 = cx + (obj.x2 || 0); + let y2 = cy + (obj.y2 || 0); if (digits !== undefined) { - return [ - converter(parseFloat(Number(obj.x1).toFixed(digits)), parseFloat(Number(obj.y1).toFixed(digits))), - converter(parseFloat(Number(obj.x2).toFixed(digits)), parseFloat(Number(obj.y2).toFixed(digits))), - ]; + 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(obj.x1, obj.y1), - converter(obj.x2, obj.y2), - ]; + return [converter(x1, y1), converter(x2, y2)]; } fromPointArray(points, deconvertor) { diff --git a/plugins/annotations/annotationsGUI.js b/plugins/annotations/annotationsGUI.js index 156d9c1d..eef5760b 100644 --- a/plugins/annotations/annotationsGUI.js +++ b/plugins/annotations/annotationsGUI.js @@ -2286,7 +2286,7 @@ class="btn m-2">Set for left click ` if (unknown.length) { groups.push({ name: "Unknown / Unsorted", - presets: unknown + presets: unknown.map(pId => presetManager.get(pId)) }); } return groups;