Structure_Engine: Fix stack overflow exception for Area for FEMesh #3330
Conversation
Many behind the upgrade from removing the Node name from start end performed in #3236 Simply re-serialising all of them to make sure they are up to date and do not need versioning to deserialise
Issue stemming from c65b01f where the call to extract the geometry from the FEMesh was removed, and the method instead was changed to simply call itself. Correcting namespace of Elements0D for the pile class as that was assigned wrong namespace, which in turn was making it impossible to reach the code for extracting the geometry for the FEMesh
IsakNaslundBh
requested review from
rwemay,
al-fisher,
adecler and
FraserGreenroyd
as code owners
|
@IsakNaslundBh to confirm, the following actions are now queued:
|
|
@BHoMBot check unit-tests |
|
@IsakNaslundBh to confirm, the following actions are now queued:
|
|
@BHoMBot check unit-tests |
|
@FraserGreenroyd to confirm, the following actions are now queued:
There are 2 requests in the queue ahead of you. |
downgrading again due to some serialisation issues
|
@IsakNaslundBh to confirm, the following actions are now queued:
|
…sues. Difference showcased affects 12 significant figure, hence no real difference
UTs started failing after change of IsPlanar methodology in #3298 For this, the panels have been made planar by projected them to their plane. Panels failing where out of plane by ~2e-6, hence tests should still be valid.
Raised issue #3335 to fix
|
@IsakNaslundBh to confirm, the following actions are now queued:
|
|
Have tried to get all structures UTs working as intended now. The only one still failing locally is the Cellular section, and that is a serialisation issue rather than a UT issue. It is coming from the change to how the name is handled in #3144 . Where the obejct returned has an empty name, but when serialised and de-serialised it comes back with a null name. Might be worth raising an issue for, not sure. Could potentially set names on deserialization to emtpy strings rather than null, but that could cause similar issues the other way around. Other otption would be to roll back the change for the name made in the linked PR, or make it onyl skip name if null, rather than NullOrEMpty. |
|
The Bot seem to agree with me. Think this can be reviewed now. Only failing UT for structures is the one I mentioned above. The rest is also to be fixed, but not in this PR, but should be fixed by the corresponding code owners, which is @IsakNaslundBh , @alelom @peterjamesnugent @FraserGreenroyd (See current failing tests) |
There was a problem hiding this comment.
Happy to approve following the review session with @IsakNaslundBh
|
@IsakNaslundBh to confirm, the following actions are now queued:
|
|
The check |
|
The check |
|
FAO: @FraserGreenroyd The check they wish to have dispensation on is unit-tests. If you are providing dispensation on this occasion, please reply with:
|
|
@BHoMBot this is a DevOps instruction. I am authorising dispensation to be granted on check ref. 24100093353 |
|
@IsakNaslundBh I have now provided a passing check on reference |
|
Providing dispensation as of comment here: #3330 (comment) |
|
@BHoMBot check ready-to-merge |
|
@IsakNaslundBh to confirm, the following actions are now queued:
|
Issues addressed by this PR
Closes #3323
Found by investigating issue with unit tests. Problem was a real stack overflow exception in the area method for the FEMesh.
All structural dataset re-serialised to account for the change from StartNode to Start etc on the bars.
Test files
Unit-tests should now run.
Changelog
Additional comments