Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SofaKernel] Some Topology cleaning... #866

Merged
merged 7 commits into from Jan 10, 2019

Conversation

5 participants
@epernod
Copy link
Contributor

commented Dec 13, 2018

Starting the work of cleaning the topology in Sofa.. resting in peace since 2012...

  • Change those methods: now returns unsigned int instead of int:
    • getEdgeIndex(PointID v1, PointID v2);
    • getTriangleIndex(PointID v1, PointID v2, PointID v3);
    • getQuadIndex(PointID v1, PointID v2, PointID v3, PointID v4);
    • getTetrahedronIndex(PointID v1, PointID v2, PointID v3, PointID v4);
    • getHexahedronIndex(PointID v1, PointID v2, PointID v3, PointID v4, PointID v5, PointID v6, PointID v7, PointID v8);
  • Now unfound ID == UINT_MAX instead of -1.
  • Change several serr/sout into msg_

In this PR, 2 scene tests are now sending error due to the conversion of serr into msg_error.
This is beacuse some scene test try to do topological change on regularGrid which doesnt support that. I would like to keep those scene error and handle that in a next PR (to split PRs and simplify review process)


This PR:

  • builds with SUCCESS for all platforms on the CI.
  • does not generate new warnings.
  • does not generate new unit test failures.
  • does not generate new scene test failures.
  • does not break API compatibility.
  • is more than 1 week old (or has fast-merge label).

Reviewers will merge only if all these checks are true.

epernod added some commits Dec 13, 2018

[SofaKernel] Change method in Topology to get edge/triangle/quad/hexa…
…/tetra index by their vertex indices to return an id (unsigned int) instead of int. If not found those methods return now UINT_MAX instead of -1.
@epernod

This comment has been minimized.

Copy link
Contributor Author

commented Dec 13, 2018

[ci-build][with-all-tests]

[examples] Add more topology modifiers scene to the non regression te…
…st. Restore RemovingTetra2TriangleProcess scene which is failing but was hidden.
@hugtalbot

This comment has been minimized.

Copy link
Contributor

commented Dec 17, 2018

A scene "RemovingTetra2TriangleProcess.scn" seems to crash on the CI
and two examples seem to have issues: HexahedronForceFieldTopologyChangeHandling.scn and QuadForceFieldTopologyChangeHandling.scn

@epernod

This comment has been minimized.

Copy link
Contributor Author

commented Dec 17, 2018

yes, it is written in the details ;)

@epernod epernod self-assigned this Dec 17, 2018

@guparan

This comment has been minimized.

Copy link
Member

commented Dec 19, 2018

Not sure about the UINT_MAX used for error handling. How about something like BaseTopologyObject::INDEX_NOT_FOUND (secretly equals to UINT_MAX)? Or an exception (cleaner)?

EDIT: There is already a Topology::InvalidID 😉

@epernod

This comment has been minimized.

Copy link
Contributor Author

commented Dec 19, 2018

yes but it ends in the same discussion as all the other typedefs that are in sofa::core::topology::Topology {}
You need to inherite or at least include that class to be able to use Topology::InvalidID ...
Maybe a more generic one in sofa::defaultTypes ?

@untereiner

This comment has been minimized.

Copy link
Contributor

commented Dec 19, 2018

Maybe put it out of the Topology class wiith something like
struct TopologyTraits{ using InvalidID = UINT_MAX; };

Then you can use it as TopologyTraits::InvalidID;
Same for the other typedefs

epernod added some commits Dec 21, 2018

[SofaKernel] Add TopologyTypes.h inside sofa::defaultType to define I…
…nvalidID = UINT_MAX. Propagate this invalidID definition inside Topology.h and all topology classes.
@untereiner

This comment has been minimized.

Copy link
Contributor

commented Dec 23, 2018

A possible option for a first move towards the use of multiple mesh data structures would be to put all the "using" out of the xxxTopologyContainer classes into a traits.

template <T> struct Cell { };
template <> struct Cell<SofaBaseMesh> { using Vertex = index_type; using Edge = index_type; .... };

The someone else could write (in a plugin)
template<> struct Cell<MyOwnMeshDataStructure> { using Vertex = another_type; .... };

@damienmarchal

This comment has been minimized.

Copy link
Contributor

commented Jan 10, 2019

Ready ?

@guparan guparan merged commit c45d13d into sofa-framework:master Jan 10, 2019

7 checks passed

Jk2/Dashboard Builds triggered.
Details
Jk2/[with-regression-tests] Triggered in latest build.
Details
Jk2/[with-scene-tests] Triggered in latest build.
Details
Jk2/centos_clang-5_options Build OK. FIXME: 0 unit tests, 5 scene tests 4 regressions
Details
Jk2/mac_clang-3.5_options Build OK. FIXME: 1 unit tests, 15 scene tests 4 regressions
Details
Jk2/ubuntu_gcc-5.4_options Build OK. FIXME: 0 unit tests, 5 scene tests 3 regressions
Details
Jk2/windows7_VS-2015_options_amd64 Build OK. FIXME: 1 unit tests, 5 scene tests 3 regressions
Details

@guparan guparan added this to the v19.06 milestone Jun 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.