Systems-Modeling / SysML-v2-Pilot-Implementation

Proof-of-concept pilot implementation of the SysML v2 textual notation and visualization
GNU Lesser General Public License v3.0
128 stars 24 forks source link

ST6RI-508 Nested coordinate frames for composite structure #354

Closed hpdekoning closed 2 years ago

hpdekoning commented 2 years ago

This PR adds CoordinateFrame as a specialization of VectorMeasurementReference, an abstract CoordinateTransformation (attribute def) and three concrete specializations of CoordinateTransformation, namely CoordinateFramePlacement, TranslationRotationSequence, AffineTransformationMatrix, to the UnitAndScales domain library.

A fourth transformation by quaternions will be captured in a JIRA issue for future implementation after Quaternion is added to ScalarValues.kerml.

An example on use of the coordinate frame transformations and nesting is given in: VehicleGeometryAndCoordinateFrames.sysml in sysml/src/examples/Geometry Examples. Also example CarWithShapeAndCSG.sysml has been updated with quantified shapes for a simplistic geometric model of the engine as a rectangular box with two cylindrical holes, using the currently available CSG construct via differenceOf.

There remain a number of points to be resolved in a next issue:

himi commented 2 years ago

It looks it takes an approach to explicitly specify a source coordinateFrame for nested coordinate transformation. However, in VehicleGeometryAndCoordinateFrames, vehicle.chassis.coordinateFrame.transformation does not specify it but the other parts such as *wheels do that. I guess it should specify nestedCoodinateFrame by subsetting datum if I understand you intention correctly.

himi commented 2 years ago

And may I ask to merge the latest master for further review? This version is too old and painful for me to evaluate.

hpdekoning commented 2 years ago

"It is probably too late to fully review this for inclusion in 2022-03"

Yes, agreed. In my assessment the current library is really complicated to use and explain to normal users. I really think we need to find ways to simplify, at least the user experience.

"The change to SpatialItem doesn't work. See comment on ST4MD-382."

I meant to add self to the union but could not find expression syntax to do it. Since it is a Set I am assuming that adding an item more than once to the set is idempotent. I think then unionsOf would represent the full spatial extent defined by a complete assembly tree. The redefinition of unionsOf could also be named compositeSpatialExtent or something similar (instead of componentUnion). Could we do something like:

    attribute compositeSpatialExtent[1] :> unionsOf {
        item :>> elements = self + spatialSubitems;
    }

or

    attribute compositeSpatialExtent[1] :> unionsOf {
        item :>> elements = self, spatialSubitems;
    }
seidewitz commented 2 years ago

I really think we need to find ways to simplify, at least the user experience.

This is beyond the scope of ST6RI-508, and this PR is not the best place to discuss changes beyond the implementation of 508. Let’s focus first on getting the coordinate frame update completed, and then we can separately discuss and prioritize further changes.

hpdekoning commented 2 years ago

It looks it takes an approach to explicitly specify a source coordinateFrame for nested coordinate transformation. However, in VehicleGeometryAndCoordinateFrames, vehicle.chassis.coordinateFrame.transformation does not specify it but the other parts such as *wheels do that. I guess it should specify nestedCoodinateFrame by subsetting datum if I understand you intention correctly.

In the updates the source coordinate frame was added in all example usages. In the future I hope to add a default value to source being the coordinateFrame of the owning item usage of an owned composite item usage (i.e. one level down in the composite structure). For this we need something like owningItem : Item[0..1] the inverse of subitems : Item[*] in the specification of Items::Item.

hpdekoning commented 2 years ago

And may I ask to merge the latest master for further review? This version is too old and painful for me to evaluate.

One of the last master commits before the 2022-03 release was merged into this branch to check consistency and ease review.

hpdekoning commented 2 years ago

The TODO for TranslationRotationQuaternion has been removed. Issues https://openmbee.atlassian.net/browse/ST6RI-535 and https://openmbee.atlassian.net/browse/ST6RI-536 have been created to capture the need.

seidewitz commented 2 years ago

I have pushed a commit that resolves the additional comments I posted above. I have also copied the updated library models to the library directories used by the KerML and SysML Xpect tests (these unfortunately still need to be copied by hand).

With these changes, I can approve this PR.

hpdekoning commented 2 years ago

Upon rereading definitions of innerSpaceDimension and outerSpaceDimension corrected minor typos. (And due to editor used, also removed trailing whitespace.)

OK, to merge for me.