Rename all components to have "Component" suffix

(Joël Lupien) #1

Concerns this PR:

Original proposition: https://github.com/amethyst/amethyst/issues/1666

What is the opinion of everyone on the subject?

Is the extra verbosity worth the clarity? Is there alternative solutions that can provide the same benefits (ie listing all components in the docs?)

1 Like
#2

For me the best documentation is you don’t need to document it to be clear, adding a Component and the end doesn’t hurt anyone I think, so I like the idea, is the same that all systems end with System and it’s adviced in the book to add State to all states at the end.

2 Likes
(Azriel Hoh) #3

I’ve got a moderate “no”, though the ideal alternative I wish for doesn’t exist – the alternate being rustdoc listing all types that impl Component.

Some concerns:

  • Verbosity to recognize what something is
  • Consistency with variable naming
  • Ease of refactoring (find and replace)
  • Getting people to do this in PRs (no tooling support – generation / verification)
// Component value
let transform = Transform::new();
let transform_component = TransformComponent::new();

// Storage
let transforms = world.read_storage::<Transform>();
let transform_storage = world.read_storage::<Transform>();
let transform_component_storage = world.read_storage::<TransformComponent>();

// SystemData; here rustfmt will begin to line wrap.
// There's only 2 storages read in this example. Up to 5 + other resources is not rare.
let (transforms, velocities) = world.system_data::<(Transform, Velocity)>();
let (transform_storage, velocity_storage) = world.system_data::<(Transform, Velocity)>();
let (transform_storage, velocity_storage) = world.system_data::<(TransformComponent, VelocityComponent)>();
let (transform_component_storage, velocity_component_storage) = world.system_data::<(TransformComponent, VelocityComponent)>();
(Andrea Catania) #4

Thanks for your feedback, in my mind you can omit to add the extra component word to the variable name since the type already clearly tell you what you are dealing with.

My idea is to make clear what each type is in order to make easy to read the code to anyone, and so avoid miss understanding.

The verbosity is not always necessary and good but this time I think that it worth.

(Brian West) #5

I commented on the PR itself before, but I’ll chime in here. I like the idea because I have personally had trouble on occasion looking through the rustdoc and identifying what is usable as a component and what is not. One instance of this was trying to identify a way that I could fade the transparency of an object without needing a separate sprite for it, which was the Rgba component previously which I believe has been renamed to Tint in master. It was not readily apparent looking through the doc that it was a component or usable as such.

Although I am not an expert at Rust, the impression that I have gotten is that idiomatic Rust is being verbose on type and variable names, so this would be along those same lines. That said, I would propose having backward compatible type aliases for the existing types to ease the transition. Enforcing it down the line definitely is challenging though, and there is a risk that something might go unseen because people start to prematurely ignore something that doesn’t contain Component in the name.

But, I can see a few caveats with it - one is that it assumes that creating a component corresponds to creating a new struct for it, which is not necessarily the case. I believe we have a few things that can serve in multiple capacities either as a component or as a prefab or something else. I forget exactly where.

We also have places where something initially was not a component and had an impl Component added later. It looks like Rgba was one of those. I think that would be annoying to have to rename a struct that was not originally a component just because you added an impl. Maybe a better approach would be to leave the struct name alone and then add a type alias along with the impl to append Component to the end?

1 Like
(Joël Lupien) #6

Quick note: We do not care for backward compatibility until we reach the 1.0 release. Making breaking naming changes is fine.

1 Like
(Andrea Catania) #7

If something, later on, became a component, the fact that the suffix “component” is added make easy to recognize it as component. Don’t you think?

(0x6273) #8

I think another option to clear up the type ambiguity is to pub use all of a crate’s components inside a components module instead of directly at the top of the crate.

So use amethyst::core::Transform; would become use amethyst::core::components::Transform;.

4 Likes
(Azriel Hoh) #9

Having the Component suffix makes it easier for onboarding; after onboarding, it becomes noise – the kind of noise that sometimes you want on, and sometimes you want off (kind of like debug logging).

Something to consider, Handle<AnAsset> is a Component as well. An asset handle is a Handle in its own domain. Whether it has an impl Component or not doesn’t (or shouldn’t) affect its naming – otherwise it would be HandleComponent<Asset>.

  • If this is an exception, how far would exceptions to the rule go?
  • If the type comes from another library that isn’t Amethyst, should we re-export the type under an alias?
  • What if something is a Component, then later on isn’t? (backwards compatibility breakage, if we don’t want the type name to be a lie)

The way I’ve mitigated the discovery issue for my project (75 crates, non-trivial) is through namespacing, and having a known (shallow) structure, so that imports are always use something::play::TheComponent, and I just have to look in any *::play module to find available Components. The shallow module structure (achieved through re-exports) makes it ergonomic to import.


In terms of solutions, would a tool that runs over the repo, discovers all Components, and then publishes the list somewhere suffice?

2 Likes
(Andrea Catania) #10

If the handle is a component why do you think it should not be clear that it is a component by avoiding changing its name?

However I’m always more convinced that this change is a good thing because if you read the code and you see a never meet before structure (for example the HandleComponent) you are 100% sure that it’s a component and you don’t need to search it in docs what it is.

Also by discarding this kind of addition the code became more and more cryptic where only the initial core developers know each part of the engine, and all other should check the doc each row of code.

Indeed I believe that this should be reflected to all the structures of the engine in order to have a clear map of what it is what.

Especially in an ECS architecture (where you have at least 3 time the same name for each part) programmed not using OO to me is necessary have this kind of naming rule otherwise the code will be unclear fast.

Then look at this scenario where you have other parts that doesn’t have a clear meaning like the Resources. For example you meet this:

let t = Texture::new();

What this line mean? I’m creating a component or a resource, or what??

Instead with

let t = TextureRes::new()

is clear at the same way to both the last arrived dev and the older core dev, that it is a resource an nothing more.

So if the verbosity is something that you don’t like I want to propose to cut part of the “Component” word in order to have only “Comp” as suffix HandleComp, TransformComp, and so on… .

(Théo Degioanni) #11

A resource can be any structure whatsoever. I do not believe putting it in the name would be appropriate here (we would quickly need to have Res everywhere).

Are you sure knowing wether a struct is a component or not before it is appended to a storage is relevant to the understanding of the code? Say I have a component that is immediately inserted or read, then I can assume it is a component from the storages. But if it is not inserted immediately, it is just a structure containing data (that will happen to be used as a component): Transform describes a transformation, SpriteRender describes how a sprite will be rendered, etc…

Ideally, we could solve that issue with additional tooling, too. If we had a VSCode plugin, or equivalent, tailored to Amethyst, we could have it display components and all more easily. But that’s a long way off.

2 Likes
(Andrea Catania) #12

Yes but the name of the structure must tell what that structure is mean for.

What I mean is that if you have a Transform, even if it’s not yet registered it remains a component.
Said in other word if you have a Texture resource it can’t be registered as component because it’s a resource. This mean that it require a component to use the resource.

This naming strategy enforces a clear cut between all the parts inside the engine, improving the design and the cleanness of the software.

In OO this kind of order is easy to maintain because you look at it as objects while in an non OO environment (like Rust / C) everything can be everything.
While this is fine from the prospective of the language, I think that this is deleterious for the engine understanding where soon you will mix components with resources with others in an not-understandable mess.

I strongly believe that the code must document it self, and if you need an external tool to make a bit of clarity mean that the design of the engine has some problems.

(Théo Degioanni) #13

But Rust fundamentally does not work like OO. Traits are additive, they do not define the structure itself but the interfaces it has. If something is a component, this is not a fundamental property of it, but just a way of saying “you can use that as a component”.

The data is separated from the usage in Amethyst, and I think not considering structures as strictly components would help it having a positive effect.

Does that make sense?

2 Likes
(Andrea Catania) #14

Yes and this is a key point that put in this way I can only say: Yes this addition is useless and bad.

But the thing that I want to highlight is that we need more meaning in the names.

For example, can you tell me what this structs are for:

This: https://github.com/amethyst/amethyst/blob/master/amethyst_rendy/src/camera.rs#L12
and this: https://github.com/amethyst/amethyst/blob/master/amethyst_rendy/src/camera.rs#L126
and this: https://github.com/amethyst/amethyst/blob/master/amethyst_rendy/src/camera.rs#L225
and this: https://github.com/amethyst/amethyst/blob/master/amethyst_rendy/src/camera.rs#L338

As you can see you have a bunch of names that doesn’t tell you much, I don’t know if they are components, resources, just functionalities, or any other structure that have additional descriptive data to add to the main structure.
I know that this is fine in Rust, but in practice is also messy.

What I think is that my proposal can be rejected and I’m fine with this if we could take in consideration to rethink the naming strategy in order to put meaning in the structures.

This mean that we could have structures that serve as component / resources without a descriptive suffix. If we define a rule that only these kind of structure can not have a suffix, this allow anyone to differentiate them to the other structures.

By doing so, in my mind when I read Spline in a part X of the code, I clearly know that it is something that holds only data and can be a component or a res or both. And at the same time I know that it’s not something other.

(Théo Degioanni) #15

In my opinion the example you gave really show that we lack inline documentation. Would they have had doc, the meaning would have been clear either from the code or with a quick lookup in the API reference (but please also note that those examples are from rendy, which is a newly merged thing and does not have complete doc yet).

(Andrea Catania) #16

Inline doc for sure help, but is not enough. because it works only if you see the implementation file.

Instead I would like be able at a glance to differentiate them from a file where I use them.

A good proposal from @azriel91 from the chat is to put C, and following this idea I can say that even D could be nice (D = Data) to untie from the Component meaning.

(Azriel Hoh) #17

Just to clarify, I don’t intend to include a suffix / prefix, it was more of a passing comment how the ? operator is explicit, yet concise.

(Andrea Catania) #18

Yep that still a good thing to highlight, that even rust is really explicit. (see for example the module definition using mod.rs)

#19

I’m not a fan of this change. If this is really something people want to force through then in my opinion it should be done on a namespace level, as previously suggested, instead of using suffixes.

3 Likes
(doomy) #20

This is becoming my stance as well. I’m not entirely opposed to the Component suffix, but after some excellent points brought to light by both sides, I think - if we were to change something here - the best course of action would be a re-export to a components namespace. I can see a literal suffix getting users pinned into a corner at times.

Something like a core/commonly used components prelude could be a really nice quality of life improvement.