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

[IRC] Add a Link for the mandatory FixedConstraint #152

Merged
merged 3 commits into from
Aug 26, 2024

Conversation

fredroy
Copy link
Contributor

@fredroy fredroy commented Jul 10, 2024

First reason to do that was to avoid crashing shamelessly if one did not put a FixedConstraint in the same node...

@fredroy fredroy added pr: fix pr: status to review To notify reviewers to review this pull-request labels Jul 10, 2024
Copy link
Contributor

@alxbilger alxbilger left a comment

Choose a reason for hiding this comment

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

I like the idea of the PR, but I don't like to create the fixed constraint automatically. Also, to note that the PR is breaking

context->addObject(fixedConstraint);
fixedConstraint->addConstraint(0);

l_fixedConstraint.set(fixedConstraint);
Copy link
Contributor

Choose a reason for hiding this comment

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

what about initialization of the FixedProjectiveConstraint? Even if you call it, FixedProjectiveConstraint::init can also fail, and there is no automatic mechanism to prevent invalidity of the scene. So, I am in favor of triggering an error without backup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Problem is, if you dont create a fixedconstraint (i.e is null), as the IRC code is... let's say, not really solid 🤔, it will crash later on (not test on pointer here and there etc)
The creation here of FixedConstraint is not really intended to serve as a backup actually ; more as a way to not crash by itself 😅 Thats why there is a msg_error (and not a warning).
Or instead of crashing (if we dont do anything) we can do a fatal() or something like that... a graceful exit

And as for the breaking, I agree and I assume that I wont do deprecation and stuff 🙊

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with both of you : @alxbilger for the analysis and @fredroy about the IRC component

Hard to decide which would be the better option.
I would vote for the simplest one (thinking about our future us debugging) : just an error, even if this leads to crashes (why not the fatal if it is slightly nicer)

Copy link
Contributor

Choose a reason for hiding this comment

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

To me the component should be emit an error and be set in invalid state.
Then the component state should be checked if it is valid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To me the component should be emit an error and be set in invalid state. Then the component state should be checked if it is valid.

This is what I did few lines after (commented) but only a few functions in IRC does the componentState testing.
And I dont really want to update all the functions to add the test 🥸

@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 Jul 17, 2024
@fredroy fredroy 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 Aug 19, 2024
@bakpaul bakpaul 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 Aug 21, 2024
@bakpaul bakpaul merged commit 470eeab into sofa-framework:master Aug 26, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: fix pr: status ready Approved a pull-request, ready to be squashed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants