Skip to content

Conversation

@Eideren
Copy link
Collaborator

@Eideren Eideren commented May 7, 2025

PR Details

Constraints with Quaternion values are not initialized with valid values, this PR ensures that they are initialized to Identity.
Also adds a couple of summaries.

N.B.: I mistakenly made it as a new branch under stride, I'll delete it as soon as I'm done

Related Issue

None reported

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My change requires a change to the documentation.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have built and run the editor to try this change out.

public sealed class TwistLimitConstraintComponent : TwoBodyConstraintComponent<TwistLimit>, ISpring
{
public TwistLimitConstraintComponent() => BepuConstraint = new() { SpringSettings = new SpringSettings(30, 5) };
public TwistLimitConstraintComponent() => BepuConstraint = new()
Copy link
Member

@Kryptos-FR Kryptos-FR May 7, 2025

Choose a reason for hiding this comment

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

I must say, I'm not a fan of the expression body syntax for methods in general and even less for constructors. IMO it should be reserved for property accessors on in rare case when a method redirect to another (e.g. for wrapping types).

Copy link
Contributor

Choose a reason for hiding this comment

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

I used them for small methods like these which are easy to read and it removes a few lines. I mean, the readability is still ok 🙂.

Copy link
Member

Choose a reason for hiding this comment

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

I guess I'm not used to it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not too keen on it either, but felt that in this case it was warranted as those classes are effectively just wrappers around bepu's constraints. If I were to change that, I would have to change the ~30 other constraint classes, let me know.

Copy link
Member

Choose a reason for hiding this comment

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

It was just a remark on my side. I guess it's a personal preference and it doesn't warrant a change in this PR

@Eideren Eideren merged commit 63e4207 into master May 10, 2025
2 checks passed
@Eideren Eideren deleted the constraints_default_fix branch May 10, 2025 12:45
@delustra
Copy link

That fixes #2680

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants