Skip to content

[SofaCore] Inf fix topo subset indices#2870

Merged
hugtalbot merged 3 commits intosofa-framework:masterfrom
epernod:inf_fix_topoSubsetIndices
May 13, 2022
Merged

[SofaCore] Inf fix topo subset indices#2870
hugtalbot merged 3 commits intosofa-framework:masterfrom
epernod:inf_fix_topoSubsetIndices

Conversation

@epernod
Copy link
Copy Markdown
Contributor

@epernod epernod commented Apr 14, 2022

this PR is based on #2869

Sevral fixes in TopologyData / TopologySubsetData / TopologySubsetIndices

  • Fix: TopologyData::m_lastElementIndex was set to invalid when passing empty buffer size. keep value to 0 in this case.
  • Rename: TopologySubSetData option isConcerned by addNewelements. This method is used to allow TopologySubsetData to growth following topological changes
  • Fix: TopologySubSetData add methods. Value where not really set but using default constructor instead of value in callback.
  • Clean: TopologySubSetData remove method from unecessary checks/loop

By submitting this pull request, I acknowledge that
I have read, understand, and agree SOFA Developer Certificate of Origin (DCO).


Reviewers will merge this pull-request only if

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

@epernod epernod added pr: fix Fix a bug pr: status wip Development in the pull-request is still in progress pr: do not squash To merge instead of squash a pull-request labels Apr 14, 2022
@epernod epernod force-pushed the inf_fix_topoSubsetIndices branch from e6dc9e9 to 7330cc2 Compare April 27, 2022 11:58
@epernod epernod added pr: status to review To notify reviewers to review this pull-request and removed pr: status wip Development in the pull-request is still in progress labels Apr 27, 2022
@epernod
Copy link
Copy Markdown
Contributor Author

epernod commented Apr 27, 2022

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

@epernod epernod added pr: status wip Development in the pull-request is still in progress and removed pr: status to review To notify reviewers to review this pull-request labels Apr 28, 2022
@epernod epernod force-pushed the inf_fix_topoSubsetIndices branch 2 times, most recently from 7985433 to 7f7d940 Compare April 28, 2022 23:57
@epernod epernod added pr: status to review To notify reviewers to review this pull-request and removed pr: status wip Development in the pull-request is still in progress labels Apr 28, 2022
@epernod
Copy link
Copy Markdown
Contributor Author

epernod commented Apr 28, 2022

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

1 similar comment
@epernod
Copy link
Copy Markdown
Contributor Author

epernod commented May 2, 2022

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

@hugtalbot hugtalbot changed the title Inf fix topo subset indices [SofaCore] Inf fix topo subset indices May 4, 2022
@epernod
Copy link
Copy Markdown
Contributor Author

epernod commented May 5, 2022

@hugtalbot any update on your suggestion for the method name?

… (-1) when passing empty buffer size. Set to Invalid in this case. Update TopologyData to better update this index while adding/removing elements
@epernod epernod force-pushed the inf_fix_topoSubsetIndices branch from 7f7d940 to b8abe99 Compare May 11, 2022 07:17
* By default @sa m_addNewElements is set to false.
* @param {bool} to change m_addNewElements value.
*/
void supportNewElements(bool value) { m_addNewElements = true; }
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
void supportNewElements(bool value) { m_addNewElements = true; }
void supportNewElementsInTopology(bool value) { m_addNewElements = true; }

Copy link
Copy Markdown
Contributor

@hugtalbot hugtalbot May 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expliciting that we refer to the topology in the name can be very useful since this function is actually used in the components (e.g. FF etc)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you like it @epernod ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

supportNewElementsFromTopology ?
as reminder the definition is: "If set to true, new element created in the topology will be added in this subsetData which depends on this topology.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you like it @hugtalbot ? :)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer the shorter version (In) but it's anyway better with the keyword topology

epernod added 2 commits May 13, 2022 15:48
…ddNewTopologyElements. This method is used to allow subsetData to growth follow topological changes
…ot set. Clean remove method from unecessary checks/loops. Better tracking of Data LastElementIndex
@epernod epernod force-pushed the inf_fix_topoSubsetIndices branch from b8abe99 to 2c1a226 Compare May 13, 2022 13:54
@hugtalbot hugtalbot added pr: status ready Approved a pull-request, ready to be squashed and removed pr: status to review To notify reviewers to review this pull-request labels May 13, 2022
@hugtalbot hugtalbot merged commit aff54d5 into sofa-framework:master May 13, 2022
@epernod epernod deleted the inf_fix_topoSubsetIndices branch May 13, 2022 18:55
@guparan guparan added this to the v22.06 milestone Jun 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr: do not squash To merge instead of squash a pull-request pr: fix Fix a bug pr: status ready Approved a pull-request, ready to be squashed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants