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.

Sign up for GitHub

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

Jump to bottom

[SofaSimpleFem] ADD computeBBox and test to HexaFEMForceField #289

Merged
merged 3 commits into from
Jun 30, 2017
Merged

[SofaSimpleFem] ADD computeBBox and test to HexaFEMForceField #289

merged 3 commits into from
Jun 30, 2017

Conversation

untereiner
Copy link

@untereiner untereiner commented Jun 6, 2017 •
edited by damienmarchal
Loading

add bounding box to hexaFemForceField

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.

@guparan guparan added pr: fix Fix a bug pr: status to review To notify reviewers to review this pull-request labels Jun 6, 2017
@guparan guparan changed the title Fix bb hexafem ADD computeBBox and test to HexahedronFEMForceField Jun 6, 2017
@guparan guparan changed the title ADD computeBBox and test to HexahedronFEMForceField [SofaSimpleFem] ADD computeBBox and test to HexaFEMForceField Jun 6, 2017
@guparan guparan added enhancement About a possible enhancement and removed pr: fix Fix a bug labels Jun 6, 2017
@damienmarchal
Copy link
Contributor

I like new tests so thanks for your PR.

@damienmarchal damienmarchal self-assigned this Jun 6, 2017
@hugtalbot hugtalbot 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 Jun 7, 2017
@guparan
Copy link
Contributor

guparan commented Jun 7, 2017

Rebased and cleaned. onlyVisible restored.

@guparan guparan 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 Jun 7, 2017
hugtalbot
hugtalbot approved these changes Jun 7, 2017
View reviewed changes
SofaKernel/modules/SofaSimpleFem/HexahedronFEMForceField.h Outdated
@@ -343,6 +343,8 @@ class HexahedronFEMForceField : virtual public core::behavior::ForceField<DataTy
void addKToMatrix(const core::MechanicalParams* mparams, const sofa::core::behavior::MultiMatrixAccessor* matrix);


void computeBBox(const core::ExecParams* params, bool);
Copy link
Contributor

Choose a reason for hiding this comment

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

missing bool name onlyVisible and if false return

@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 Jun 7, 2017
@damienmarchal
Copy link
Contributor

[ci-build]

@guparan guparan added pr: status wip Development in the pull-request is still in progress and removed pr: status ready Approved a pull-request, ready to be squashed labels Jun 12, 2017
@guparan
Copy link
Contributor

guparan commented Jun 12, 2017 •
edited
Loading

This PR is not ready, it seems to mess with the CI. See all latest failing builds (239 => 243) on windows7_VS-2015_amd64_pr_1_options.

matthieu-nesme
matthieu-nesme suggested changes Jun 14, 2017
View reviewed changes
Copy link
Member

@matthieu-nesme matthieu-nesme left a comment

Choose a reason for hiding this comment

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

minor change required

SofaKernel/modules/SofaSimpleFem/HexahedronFEMForceField.inl Outdated
helper::ReadAccessor<DataVecCoord> x = this->mstate->read(core::VecCoordId::position());

static const Real max_real = std::numeric_limits<Real>::max();
static const Real min_real = std::numeric_limits<Real>::min();
Copy link
Member

Choose a reason for hiding this comment

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

warning std::numeric_limits::min() is not what you want, you want either -std::numeric_limits::max() or in c++11 std::numeric_limits::lowest()

@guparan guparan 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 Jun 20, 2017
@damienmarchal
Copy link
Contributor

Hi all,

Is this PR ready ?

@untereiner
Copy link
Author

Do you agree @matthieu-nesme ?

@hugtalbot
Copy link
Contributor

[ci-build]

@damienmarchal
Copy link
Contributor

This PR fullfill our merging rules...so I pass it to ready.

@damienmarchal damienmarchal 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 Jun 30, 2017
@hugtalbot hugtalbot merged commit acea897 into sofa-framework:master Jun 30, 2017
@guparan guparan added this to the v17.12 milestone Jul 3, 2017
@untereiner untereiner deleted the fix_bb_hexafem branch July 21, 2017 10:10
matthieu-nesme pushed a commit to Anatoscope/sofa that referenced this pull request Aug 4, 2017
@matthieu-nesme
Merge pull request sofa-framework#289 from mimesis-inria/fix_bb_hexafem
33ddebc 
[SofaSimpleFem] ADD computeBBox and test to HexaFEMForceField
@guparan guparan mentioned this pull request Sep 19, 2017
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthieu-nesme matthieu-nesme matthieu-nesme approved these changes

@hugtalbot hugtalbot hugtalbot approved these changes

Assignees

@guparan guparan

@damienmarchal damienmarchal

Labels
enhancement About a possible enhancement pr: status ready Approved a pull-request, ready to be squashed
Projects
None yet
Milestone
v17.12
Development

Successfully merging this pull request may close these issues.
5 participants
@untereiner @damienmarchal @guparan @hugtalbot @matthieu-nesme