Add/Replace/Remove vs Set/Unset

[sschmid]

Overview

entity.AddHealth(100);

entity.ReplaceHealth(200);

entity.RemoveHealth();

vs

entity.SetHealth(100);
entity.UnsetHealth();

---

The code generator currently produces methods like this:

entity.AddHealth(100);

entity.ReplaceHealth(200);

entity.RemoveHealth();

As we know in Entitas exceptions are thrown when

• component is added twice
• component is removed, entity did'n have this component

You will get nice exception description like

Cannot add component 'Health' to Entity_123!
You should check if an entity already has the component before adding it or use entity.ReplaceHealth()."

While working on the new code generator using C# source generators #1005, I started using C# nullables and I was wondering if we should get rid of the exceptions and move the responsibility to the dev.

This would mean adding components even if they already exists will result in overwriting them, basically exactly what ReplaceComponent() does right now. Attempting to remove a component that doesn't exist would also NOT fail.

To communicate this fact, we can rename the methods to Set and Unset, see examples in unit tests:
https://github.com/sschmid/Entitas/blob/set-unset/tests/Entitas.Generators.IntegrationTests/ComponentTests.cs#L18-L55

[Fact]
public void ReplacesComponent()
{
    var entity = _context.CreateEntity();
    entity.SetPosition(1, 2);
    entity.SetPosition(3, 4);

    var position = entity.GetPosition()!;
    position.X.Should().Be(3);
    position.Y.Should().Be(4);
}

[Fact]
public void GettingNonExistingComponentReturnsNull()
{
    var entity = _context.CreateEntity();
    entity.GetPosition().Should().BeNull();
}

Entitas won't throw exception and communicates that using nullables.

Example:

if (entity.GetHealth() is { Value: <= 0 } health)
{
    entity.SetDestroyed();
}

Keep in mind that removing exceptions also mean no more Cannot add component 'Health' to Entity_123!

What do you think?

[rubenwe]

Personally, I would rename ReplaceComponent() to SetComponent(). To me, replace implied I should not use it if the component is not already added. Set doesn't have that connotation.

I don't feel like there is a huge downside to keeping AddComponent() around.
It can help uncover some issues that would otherwise be hard to diagnose.

While being part of #1046, I feel similarly about generating additional TryGetComponent(out component) methods.
I don't see a huge downside to having them - and situationally it might be the nicest solution.

Of course, there is some bloat that can slow down the compiler a bit. But it may also be plausible to have some of these things be generated based on settings? I haven't tested this yet, especially not in Unity. They haven't upgraded their build system and switched to user-defined MSBuild projects yet, but afaik that's still part of their plan for the future. But for now, some things (like AdditionalFiles) seem to be broken, so I'm not sure if this part is viable.

[sschmid]

@rubenwe Right, using settings sounds good. I tried adding a test value using .editorconfig, but it doesn't seem to work. fb7f45f

I updated the TestHelper.cs to feed the unit tests with a test value, and that worked well and the tests successfully reads the value. But generated code in the integration test project doesn't seem to read the .editorconfig from the root folder.

Any ideas why?

[sschmid]

Ok, I found a solution, but not sure if I'm happy.

Instead of converting the global options to a custom options struct like this:

  var options = initContext.AnalyzerConfigOptionsProvider
      .Select((provider, _) => new EntitasAnalyzerConfigOptions(provider.GlobalOptions));

and later trying to retrieve the value from it (which did not work), I'm now using a slightly different approach, by using the AnalyzerConfigOptionsProvider without Select().

var options = initContext.AnalyzerConfigOptionsProvider;

and later before generating the code, I retrieve options like this by providing the component SyntaxTree, which I had to add to the ComponentDeclaration.

var entitasOptions = new EntitasAnalyzerConfigOptions(options.GetOptions(component.Syntax!.SyntaxTree));

Why do I need to provide the SyntaxTree?!? This should not affect global options, or does it? I'm slightly confused.

see commit: 9f06a26

[OctopBP]

Hey @sschmid !

Since you're rewriting this, I have two points that I think you might be interested in. I use this in my project and it makes my life much easier.

1. Method chain

I rewrote the methods so that they return an entity so that I can call the next method as a chain.
Here's how I use it:

entity
    .SetPlayer(true)
    .SetMovable(true)
    .AddView(backing.gameObject)
    .AddPosition(backing.transform.position)
    .AddMoveComplete(true)
    .AddPositionListener(backing);

This reduces the number of code and helps to group it better.

2. Option instead of null

I use LanguageExt library in my project and I also added it to my Entitas generator.
It has the type Option<T>, and it is an alternative to nullable types. But this forces devs to handle None (null) values, and also has many useful methods, as in LINQ.

For example this component, will generate two new properties for me: maybePosition and maybePosition_value

public class PositionComponent : IComponent
{
    public Vector2 value;
}

public Option<PositionComponent> maybePosition;
public Option<Vector2> maybePosition_value;

Thanks to this, I can safely use position later in my code

entity.maybePosition_value.IfSome(position =>
{
    // Its guaranteed here that this entity have position
});

Or we can do some calculations, as in this fictional example

entity.maybePosition_value
    .Map(position => position.normalized)
    .Filter(normal => normal.x > 0)
    .IfSome(positiveNormals =>
        {
            // ...			
        }
    );

Or like here:

var hasFinished = entity.maybePosition_value.Match( 
    Some: position  => position.x > 10,
    None: () => false
);

And a lot more :)

I will be happy to help if you find any of this useful for Entitas.

[sschmid]

I will resist and try not to change too much :D
I will keep the familiar api with Add / Replace / Remove for now. I would also research if I can overwrite default analyzer behaviour for nullable checks before I would attempt to migrate to nullable. E.g. in reactive system + Filter() we're guaranteed that entities have components, but the compiler doesn't know that and warns about possible null.

@OctopBP Yes! I'll definitely do the fluent api!

Thanks for your feedback! Will close