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

Added overload for Entity constructor #1499

Merged
merged 4 commits into from Dec 30, 2022
Merged

Conversation

palkercode
Copy link
Contributor

PR Details

Added overload for Entity constructor.

Description

Now you can create entities with rotation parameter

Motivation and Context

In my game with procedural generation, many prefabs of rooms are created. I wanted them to randomly rotate, but I did not find a parameter with rotation.

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.

@CLAassistant
Copy link

CLAassistant commented Aug 8, 2022

CLA assistant check
All committers have signed the CLA.

tebjan
tebjan previously approved these changes Aug 8, 2022
Copy link
Member

@tebjan tebjan left a comment

Choose a reason for hiding this comment

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

Nice one. LGTM, no objections from my side.

@manio143
Copy link
Member

manio143 commented Aug 8, 2022

Hmm, I don't know if that's the best move here - the next person will want a position and a scale and another all three - so maybe there's a way to just have one constructor with optional parameters? We could break binary compatibility here and modify the constructor that currently takes only position.

My main issue with it is how easy it is for the user to write a wrapper method that would handle setting the rotation on an entity without modifying the source code.

@VaclavElias
Copy link
Contributor

Maybe we could have an experimental/preview quality of life extension(s), and if we knew that people are using some of them heavily and frequently, and it would be highly requested (we could vote after certain period), then it could move to the core..

But for this, would be great to have some docs so these are listed and visible as experimental?

@tebjan
Copy link
Member

tebjan commented Aug 8, 2022

position and a scale and another all three

Good point, it should also have the scale parameter as an optional parameter set to 1. then you got one with position and one with all three. that should do... if it is possible to add the two parameters to the original constructor without breaking changes, I'd be for that.

Maybe we could have an experimental/preview quality of life extension(s)

This exists here, now that you mention it, I believe it has all that already: https://github.com/dfkeenan/StrideToolkit

@tebjan tebjan self-requested a review August 9, 2022 17:31
@tebjan tebjan dismissed their stale review August 9, 2022 17:31

New info

Copy link

@VoidEssy VoidEssy left a comment

Choose a reason for hiding this comment

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

Overloads are straight forward and simple so looks like a fine solution to me. Microsoft themselves use overloads all the time in their native classes.

Guide the user / coder instead of making them re-invent the bicycle.

@Eideren
Copy link
Collaborator

Eideren commented Aug 23, 2022

In general I would be against such a change but for this particular case I think it makes sense, setting an initial rotation for an entity is a fairly common operation, I do think that this overload is worth the very small amount of bloat it introduces.

We could repurpose the second constructor for this though;

public Entity(string name)
    : this(Vector3.Zero, name)
{
}

to this

public Entity(string name = null, Vector3 position = default, Quaternion? rotation = null, Vector3? scale = null)
    : this(name, false)
{
    Id = Guid.NewGuid();
    TransformValue = new TransformComponent { Position = position, Rotation = rotation ?? Quaternion.Identity, Scale = scale ?? Vector3.One };
    Components.Add(TransformValue);
}

Users would then pick and choose what they want with something like the following

new Entity(rotation:Quaternion.RotationY(180f));
new Entity("Some Entity", scale:Vector3.One * 2f);
new Entity("Some Entity", new Vector3(1f, 2f, 3f), Quaternion.RotationY(180f));

What do you think ?

@VaclavElias

This comment was marked as off-topic.

@Eideren

This comment was marked as off-topic.

@manio143
Copy link
Member

@palkercode would be up for updating this PR with Eideren's suggestion in #1499 (comment) ?

@manio143 manio143 merged commit 4edeee8 into stride3d:master Dec 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants