modelica
Improve inheritance of MultiBody forces
Refs https://github.com/modelica/ModelicaStandardLibrary/issues/3739
This is the next PR to #4227. I've also performed partial regression tests of multibody examples. No difference to reference resutls indicated.
That breaks some of the stated compatibility issues, but on the other hand it is a clear bug.
To me it seems straight forward to have R_rel as a counterpart to r_rel_b. So I would prefer to retain this new variable. In which manner does it "breaks some of the stated compatibility issues"?
That breaks some of the stated compatibility issues, but on the other hand it is a clear bug.
To me it seems stight forward to have R_rel as a counterpart to r_rel_b. So I would prefer to retain this new variable. In which manner does it "breaks some of the stated compatibility issues"?
If it is a maintenance fix it would break the rule in Modelica.UsersGuide.ReleaseNotes.VersionManagement
Introducing a new name in the public section of a class (model, package, ...) or in any section of a partial class is not allowed. Since otherwise, a user might use this new name and when storing its model and loading it with an older bug-fix version, an error would occur.
However, one could argue that it isn't a new name as it was already documented.
However, one could argue that it isn't a new name as it was already documented.
Good Idea.
Yet another possibility would be to wait with merging till there is a dedicated v4.1.0 branch.
@MartinOtter , could you please provide your review for this PR?
@tobolar, we have a cut-off date on Feb 2, if you think you can manage this issue by then, please go ahead, otherwise I'd suggest we reschedule for 4.2.0. There is no need to delay 4.1.0, which already has a lot of improvements, the next release will be within one year. Thanks!
@MartinOtter and @HansOlsson please review and finalize this PR, otherwise agree with @tobolar on shifting the milestone. Feel free to invite additional reviewers.
- This proposal is not backwards compatible: Assume PartialForce is used outside of MSL, then this will give an error, because the new Partial Force has R_rel, which an existing model of Partial Force might have in some (rare) cases.
Ok, this is reasonable.
- The new implementation is significantly less efficient: Computing a relative rotation matrix requires 27 multiplications. Furthermore two forces and torques are transformed with this matrix: In total 27+3+3 = 33 multiplications The current implementation does not compute R_rel, but uses Frames.resolveRelative, which requires 6 multiplications. Since 2 vectors (force and torque) are transformed in the "old" PartialForce, a total of 12 multiplications is used to achieve the same result (12 << 33). For pure force elements (torque = 0), R_rel is not needed, and therefore efficiency is reduced for these force elements.
Hmm. I didn't consider this. Just considered the users view of understanding, where having the relative Martix R_rel doing all the transformation is straightforward.
I will revert the introduction of R_rel and fix the documentation accordingly.