stride icon indicating copy to clipboard operation
stride copied to clipboard

Added overload for Entity constructor

Open palkercode opened this issue 1 year ago • 7 comments

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)
  • [x] New feature (non-breaking change which adds functionality)
  • [ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • [x] My change requires a change to the documentation.
  • [ ] I have added tests to cover my changes.
  • [ ] All new and existing tests passed.

palkercode avatar Aug 08 '22 11:08 palkercode

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Aug 08 '22 11:08 CLAassistant

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.

manio143 avatar Aug 08 '22 17:08 manio143

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?

VaclavElias avatar Aug 08 '22 17:08 VaclavElias

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 avatar Aug 08 '22 17:08 tebjan

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 ?

Eideren avatar Aug 23 '22 08:08 Eideren

I have got a few side questions (probably missing in our docs):

  • Is it a good practise to name the entities?
  • When you would name them?
  • When you wouldn't name them?

I would use your input for our docs. Thanks :)

VaclavElias avatar Aug 23 '22 09:08 VaclavElias

Hey @VaclavElias, this is a bit off topic, let's go over that on discord. I'll hide your message to avoid derailing the conversation.

Eideren avatar Aug 23 '22 09:08 Eideren

@palkercode would be up for updating this PR with Eideren's suggestion in https://github.com/stride3d/stride/pull/1499#issuecomment-1223764151 ?

manio143 avatar Dec 28 '22 15:12 manio143