Repository navigation
feat(migrate): honour the compartments a Cameo symbol suppresses as per-symbol DiagramLayout switches - #1197
Conversation
…agramLayout show… switches Co-Authored-By: jason.han <hanhuijun@gmail.com>
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
… and the kind map Co-Authored-By: jason.han <hanhuijun@gmail.com>
… count them apart from the clipped rows The migrator writes the switches from a Cameo symbol's own and inherited SUPPRESS_* properties; the positioned-tree renderer neither lists a hidden compartment's rows nor counts them among the rows the box's stated size clips. Fixtures: a view fixture (one kind off, all off, a cramped box, a rendering without compartments) and a minimal Cameo stream with a style chain. Co-Authored-By: jason.han <hanhuijun@gmail.com>
There was a problem hiding this comment.
Note
Newer findings are available below. Devin Review posted a newer report on this PR, in addition to the findings presented here.
Devin Review found 3 potential issues.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| if row.Referential && !row.StandIn { | ||
| return node.Geometry.Hidden.Hides("references") | ||
| } | ||
| return node.Geometry.Hidden.Hides(compartmentName(row.Kind)) |
There was a problem hiding this comment.
🟡 Suppressed kinds leave reference rows visible
When showParts = false, hiddenRow leaves a ref part visible unless showReferences is also false. The reference check bypasses the row's kind, so suppressed kind compartments still show referential members.
Learn more
A tree can put a ref part in the parts compartment, as compartmentsOf groups rows by keyword. The new reference check prevents the kind check from running. It therefore keeps referential members of a hidden kind in the box when references remain enabled.
Example: With showParts = false and showReferences unset, ref part mount : Mount remains listed in parts, while an ordinary part lens : Lens is removed.
Recommended fix: Hide a referential row when either its references switch or its own kind's switch hides it; retain the kind check after the reference check.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Intended, and documented in docs/project/diagram-layout-annotations.md: showReferences is the switch of the ref rows wherever they are listed, showParts the switch of the composite part rows. The switches mirror the tool's compartments: Cameo lists a reference property in its own "references" compartment, apart from "parts", and SUPPRESS_PARTS_COMPARTMENT leaves the references visible there. A ref part in a box whose Layout says showParts = false with showReferences unset is therefore still listed, as it was in the source diagram; a modeler who collapsed both gets showReferences = false too. Hiding it under showParts would drop a row the diagram showed.
One correction did come out of checking this: an attribute is referential by Usage::isReference, so the first revision treated attribute rows as reference rows and showAttributes = false did not hide them. referenceRow now counts only structural non-composite usages (ref part, ref item, …) as reference rows (2d43de8).
There was a problem hiding this comment.
Handled in 96e0ce3. The rule is deliberate: a ref part belongs to the references compartment (SysML v2 graphical notation; Cameo's SUPPRESS_REFERENCES_COMPARTMENT is what hides it there), so showReferences governs it and showParts does not — otherwise a Cameo symbol that suppresses parts but shows references would lose its reference rows. What 96e0ce3 fixed is the over-wide classification: referenceRow now marks only structural non-composite usages as referential, so attribute rows (which Usage::isReference also reports) follow showAttributes. Covered by TestGoldenSuppressedCompartments and TestSuppressedCompartmentsAreNotListedNorClipped (internal/ir/view; the selective view shows a ref part under showParts = false and hides it under showReferences = false).
…t places it; an attribute row is not a reference row Co-Authored-By: jason.han <hanhuijun@gmail.com>
…nt it gathers is suppressed Co-Authored-By: jason.han <hanhuijun@gmail.com>
There was a problem hiding this comment.
Note
Newer findings are available below. Devin Review posted a newer report on this PR, in addition to the findings presented here.
Devin Review found 1 new potential issue.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| for _, p := range stream.Placements { | ||
| if _, ok := compartments[p.ID]; !ok && (len(p.HiddenCompartments) > 0 || len(p.UnsupportedCompartments) > 0) { | ||
| compartments[p.ID] = p |
There was a problem hiding this comment.
🟡 Repeated symbols hide the wrong compartments
When repeated symbols share an element ID, compartments can take suppressions from a later symbol for the first export placement. The first symbol's visible rows then disappear from its box.
Learn more
A diagram can contain more than one symbol for the same model element. The symbol stream records each symbol separately in streamRecord, but the export and merge pair placements only by element ID. The new map skips placements without suppressions, so the next symbol with that ID can supply switches for a different box. placements then writes the first placement for that reference and silently drops later placements.
Example: Two Camera shapes share _blk_camera. The first has no suppressed compartments and is placed at x=50 by the export; the second suppresses parts. The merged layout at x=50 receives showParts = false, hiding the first shape's parts.
Recommended fix: Match a stream symbol to its exported box before transferring settings, using geometry or a stable symbol identifier where available. If matching is ambiguous, avoid borrowing settings from another symbol; test multiple symbols of one element with different switches.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed: the merge now transfers a stream symbol's switches onto the export's box only when every symbol of that element in the stream agrees on them (HiddenCompartments and UnsupportedCompartments equal); symbols of one element that differ leave the export's box as the export states it, so no box borrows another symbol's settings. TestSuppressedCompartmentsSurviveAnExportRecord now gives Lens two symbols that differ and an export box: the box keeps the export geometry and no switch, while the single-symbol Camera keeps its four.
There was a problem hiding this comment.
Fixed in f43e8fb (a symbol's switches reach an export's box only when every stream symbol of that element states the same hidden kinds; disagreeing symbols transfer nothing) and 66e3204 (agreement judged on the hidden kinds alone, so differing unsupported properties no longer discard agreed switches). Covered by TestSuppressedCompartmentsSurviveAnExportRecord (internal/translate/migrate) and the second Lens symbol in tests/migrate/testdata/xmi/suppressed_compartments.stream.xml, checked by TestGoldenSuppressedCompartments.
… box only when the element's symbols agree Co-Authored-By: jason.han <hanhuijun@gmail.com>
…hidden kinds alone Co-Authored-By: jason.han <hanhuijun@gmail.com>
Co-Authored-By: jason.han <hanhuijun@gmail.com>
…ment left out Co-Authored-By: jason.han <hanhuijun@gmail.com>
…ment switches Re-vendors OpenSysML Libraries from Open-MBEE/OpenSysML-Extensions-Library 5134a14 (Layout gains showAttributes, showParts, showReferences, showPorts, showConstraints, showItems, showActions), regenerates stdlib.snapshot and re-records the pilot-diff baseline's library digest; verdicts unchanged. Co-Authored-By: jason.han <hanhuijun@gmail.com>
…ameo-suppressed-compartments Co-Authored-By: jason.han <hanhuijun@gmail.com> # Conflicts: # docs/project/sysml-v1-transformation-census-baseline.json # docs/project/sysml-v1-transformation-census.md
There was a problem hiding this comment.
Devin Review found 1 new potential issue.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| * general it is drawn to, `ref :>> general : Sup;`, one Route | ||
| * per general. */ | ||
| attribute points : ScalarValues::Real[0..*] ordered nonunique; | ||
| ref general : Base::Anything[0..1]; |
There was a problem hiding this comment.
🔴 Specialization routes never reach the drawing
When a model supplies Route.general for a specialization, the tree draws its line without those waypoints. edgesOf creates relationship edges without reading routes, so each general-specific path is lost.
Learn more
The new general feature identifies which target a Route on a specific element belongs to. readRoute collects only points, while RouteOf selects a single annotation per element. edgesOf never requests a Route for its specialization edges, so it cannot use even one set of waypoints.
Example: A tree draws Child :> ParentA, ParentB with a Route about Child for each parent. Both lines remain automatically laid out instead of following their separate point sequences.
Recommended fix: Resolve Route sites by view and the general target, then attach the matching waypoints to each structural relationship edge in treeGraph.edgesOf. Retain the existing Route behavior for connections and other edges with no general.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Not this PR's change. The re-pin moves the vendored copy from the previous pin to upstream main, which also carries Route.general from Open-MBEE/OpenSysML-Extensions-Library#7. The semantic and renderer support for it (LayoutSite.General, Model.SpecializationRouteOf, the tree giving each specialization edge its Route) is #1166's, still open against develop; nothing in this PR reads general, and without #1166 the attribute is an unset optional feature, so no behaviour moves here. Leaving the thread open for the maintainer to decide the merge order with #1166.
There was a problem hiding this comment.
Confirmed cause and resolution: the re-pin brought Route.general into the vendored library while #1166's consumer code (Model.SpecializationRouteOf, LayoutSite.General, the tree's per-general routing in internal/ir/view) was not yet on this branch — #1166 merged into develop at 325735f, after this branch's previous develop merge (81777ca). Develop 325735f is now merged into this branch, so the consumer code is present; no code change in this PR.
…ameo-suppressed-compartments Co-Authored-By: jason.han <hanhuijun@gmail.com> # Conflicts: # docs/project/pilot-differential-baseline.json # internal/workspace/libs/stdlib.snapshot # scripts/extension-libraries-pin.sh
What and why
A Cameo block definition diagram migrated with
sysml -migrate sysmlkeeps each symbol's Cameo-sized box asDiagramLayout::Layout. When the modeler collapsed compartments in Cameo the box is small (a 200×33part def 'Imaging Subsystem'bar), and the positioned-tree renderer draws every compartment row into it, clips them and writes a "not represented" notice. The modeler's choice is in the diagram stream: each symbol'sSUPPRESS_*_COMPARTMENTBoolean properties, stated on the symbol or inherited from the diagram's symbol style (symbolStyles/PropertyManagerByStereotype, referenced by the symbol'ssymbolStyleID).This change honours those settings: a compartment the modeler suppressed on a symbol is written as a per-symbol
show… = falseswitch on that symbol'sDiagramLayout::Layout, and the renderers omit it for that symbol (no row, no notice). A compartment not suppressed keeps the current behaviour: drawn, clipped with a notice when the box is too small.Representation, ranked by SysML v2 spec compliance
Each candidate was probed with the pinned pilot
validate-sysml-batch(2026-08) andsysml -validate:show…attributes on the symbol's existingDiagramLayout::Layout(showAttributes,showParts,showReferences,showPorts,showConstraints,showItems,showActions; chosen) — user-defined metadata with literal Boolean values, each switch a declared attribute the language resolves, the shape of the existingcollapsedand of a rendering'sshow…attributes. Pilot clean, sysml clean. Needs the DiagramLayout library change.enum def Compartment+attribute hidden : Compartment[0..*]onLayout— valid (pilot clean, sysml clean);referencesis a keyword, so the literals cannot mirror the compartment names.attribute hidden : ScalarValues::String[0..*]onLayout— valid (pilot clean, sysml clean); compartment names are unchecked strings.rendering noParts :> asTreeDiagram { attribute showParts : Boolean = false; }) — standard-only text, valid, but view-wide. On SEM it loses per-symbol fidelity in most views: 18 of 38 views draw both symbols that suppress the class attributes compartment and symbols that show it (operations: 18, flow properties: 11, proxy ports: 11, full ports: 11, parts: 3).filterremoves an element from the whole view, not a compartment from one symbol; the standard renderings carry no compartment switches.Kind map (Cameo compartment property →
Layoutswitch)SUPPRESS_PARTS_COMPARTMENT,SUPPRESS_IBD_PARTS_COMPARTMENTshowPartsSUPPRESS_REFERENCES_COMPARTMENT,SUPPRESS_IBD_REFERENCES_COMPARTMENTshowReferencesSUPPRESS_PROPERTIES_COMPARTMENT,SUPPRESS_VALUES_COMPARTMENT,SUPPRESS_CLASS_ATTRIBUTES,SUPPRESS_IBD_VALUES_COMPARTMENT,SUPPRESS_IBD_UMLPROPS_COMPARTMENT,SUPPRESS_IBD_INITIALVALUES_COMPARTMENTshowAttributesSUPPRESS_CONSTRAINTS_COMPARTMENT,SUPPRESS_IBD_CONSTRAINTS_COMPARTMENTshowConstraintsSUPPRESS_FLOWPROPERTIES_COMPARTMENT,SUPPRESS_IBD_FLOWPROPERTIES_COMPARTMENTshowItemsSUPPRESS_PROXYPORTS_COMPARTMENT,SUPPRESS_FULLPORTS_COMPARTMENT,SUPPRESS_CLASS_PORTSshowPortsSUPPRESS_CLASS_OPERATIONS,SUPPRESS_IBD_OPERATIONS_COMPARTMENTshowActionsA v2 compartment gathers what several Cameo compartments list between them (a block's class attributes and its values both become attributes), so a switch is written only when every Cameo compartment the symbol states for it is suppressed: Cameo's own block style suppresses
SUPPRESS_CLASS_ATTRIBUTESwhile the values compartment stays shown, andshowAttributesstays unset.SUPPRESS_STRUCTURE,SUPPRESS_BEHAVIORS,SUPPRESS_IBD_BEHAVIOR_COMPARTMENTandSUPPRESS_SIGNAL_RECEPTIONSmap to no v2 compartment and are counted under "unsupported presentation properties, dropped".Migrator
sysmlv1: a symbol'sSUPPRESS_*properties are read intoSymbol.Compartments; the stream'ssymbolStylesmanagers are read with theirparentPropertyManagerchain and a symbol inherits the settings its own properties do not state, throughsymbolStyleID(a property with novalueis Cameo's default, shown, and overrides an inherited suppression).migrate:hiddenCompartmentsmaps them to kinds; the placement writer appendsshow<Kind> = falseto the symbol'sLayout; an MTIP export that places the symbol keeps the stream's switches with its box; the migration report gains# compartments suppressed: N on M symbols(compartmentsSuppressed,symbolsSuppressingin the sidecar).Renderer
semantics.Layoutreads the seven switches intoLayout.Hidden(HiddenCompartments);view.Geometrycarries it.foldCompartments) leaves a hidden compartment's rows out ofNode.Compartmentsand counts them apart in the folding notice (9 node(s) without a position: 7 drawn as compartment rows of the node above them, 2 in compartments its Layout hides left undrawn); the notice is unchanged when nothing is hidden. The clipping rule and its notice see only the compartments left on, so a hidden compartment is neither drawn nor counted as clipped; the rendering'sshowCompartments = falsestill folds nothing.showReferencesis the switch of therefrows wherever they are listed (Cameo's references compartment),showPartsof the composite part rows; an attribute row, referential byUsage::isReference, is an attribute row.Node.Compartments, so they follow with no change of their own; the text form never settled unplaced nodes and lists the members as before (documented inview-rendering-forms.md).asTreeDiagram, what a Cameo BDD migrates to). The interconnection, state, action and case renderings fold no members into compartments, so a switch written on a symbol of those views (an IBD part'sshowItems) has nothing to hide and changes nothing there.-layoutexport, a stream symbol's switches move onto the export's box of the same element when the element's symbols agree on them; symbols of one element that differ leave the export's box as it is.Behaviour changes
SUPPRESS_*value).internal/ir/view/testdata/suppressed-compartments-{selective,collapsed,cramped,roomy,bare}.{dot,mermaid,plantuml,d2,text}.golden(25) andtests/migrate/testdata/xmi/suppressed_compartments.golden.{sysml,report.txt}(2), with the fixturessuppressed-compartments.sysml,suppressed_compartments.{xmi,stream.xml,layout.xml}.uml:Class570,uml:Property571 on current develop) — re-measured, no mapping class changes.Measured on SEM.mdzip (develop vs this head)
showActions225,showItems208,showPorts141,showAttributes100,showParts66,showReferences64,showConstraints64); unsupported, dropped:SUPPRESS_BEHAVIORS202,SUPPRESS_STRUCTURE168,SUPPRESS_SIGNAL_RECEPTIONS168,SUPPRESS_IBD_BEHAVIOR_COMPARTMENT63.-portablethe output differs only by the switch clauses and the inlined DiagramLayout library text (the seven attributes and their doc comment).-render-all -render-form dot, 34 views: compartment rows reported as clipped 386 → 356 over the same 18 views. Per requested view (clipped rows before → after): Imaging BDD 26 → 23 (3 rows hidden), Subsystem Architecture 120 → 112 (8), Expanded Domain Model - Part 1 20 → 13 (7), Logical Structure 55 → 54 (1). On SEM the change is in the notices, not the picture: the Graphviz PNGs of all four views are byte-identical before and after, because every row the modeler collapsed lay in the clipped-off part of its box already; the notices now report those rows as the modeler's choice (… in compartments its Layout hides left undrawn) instead of as rows without room. The view fixture'sroomygolden shows the visible case: a box with room for every row lists everything but the collapsed parts.sysml -validate: clean. Pilotvalidate-sysml-batchon the-portableoutput: clean.'Imaging Subsystem'bar the rows still clipped are parts Cameo shows in the model but draws as composition lines rather than compartment rows; the parts compartment is not suppressed on that symbol, so the fit-and-clip rule applies to them as before. Whether a part drawn as a line should also fold into its owner's compartment is a separate decision.Specification basis
SysML v2 Language Specification (the version the pinned 2026-08 pilot implements), Metadata (§7.25): user-defined metadata definitions whose attribute values are model-level evaluable;
DiagramLayoutis the non-normative OpenSysML extension library (docs/project/diagram-layout-annotations.md). No row ofdocs/project/spec-compliance.mdmoves.How it was verified
go build ./...,go vet ./...,gofmt -l .clean;go test ./internal/translate/xmi/sysmlv1/ ./internal/translate/migrate/ ./internal/ir/view/ ./internal/semantic/semantics/ ./tests/migrate/pass against the re-pinned DiagramLayout (Open-MBEE/OpenSysML-Extensions-Library 5134a14, vendored withscripts/sync-extension-libraries.sh;stdlib.snapshotregenerated, pilot-diff baseline re-recorded for the library digest with verdicts unchanged). The re-pin moves the vendored copy from the previous pin to upstreammain, so it also carriesRoute.generalfrom feat(DiagramLayout): Route.general names the general a specialization edge is routed to OpenSysML-Extensions-Library#7; the renderer and semantic support for that attribute is fix(view): route specialization edges from Cameo and keep the keyword line in a stated box #1166's, not this PR's, and nothing here reads it.TestReadSymbolsCompartmentsInheritStyle(style chain, unvalued default, own-over-inherited),TestHiddenCompartments(kind map, all-suppressed rule, unsupported),TestGoldenSuppressedCompartments(migrate, witherrors()validation) andTestSuppressedCompartmentsSurviveAnExportRecord(MTIP join),TestGoldenSuppressedCompartmentsandTestSuppressedCompartmentsAreNotListedNorClipped(view: one kind off, all off, a cramped box, a roomy box, a rendering without compartments).transformation-census -check,scripts/check-doc-ids.py,scripts/changelog.py check,make docs-checkclean.Checklist
make testandmake lintpass locallychanges/unreleased/<slug>.<section>.md, not as an edit toCHANGELOG.mdmake docs-countsrun if a gate count moved (compliance rows need nothing: the census is counted at docs build)F4,K5) in the body, docs, or changelogLink to Devin session: https://nasa-jpl-demo.devinenterprise.com/sessions/4dd157ac12a04bf69f2b84e455f8a37b
Open in Devin Desktop: https://nasa-jpl-demo.devinenterprise.com/desktop/session/4dd157ac12a04bf69f2b84e455f8a37b?variant=devin
Requested by: @HuiJun