Proposal for #856: Deprecations instead of removals

[chapulina]

Some proposals to #856 , focused on providing a smoother migration for users with tick-tock.

• Keep ComponentKey around, it's still a unique identifier, but we use Entity as the ComponentId
• Tick-tock the StorageDescriptor classes, because they are outside of detail.
  • This also allows deprecating Factory::Register without loss of functionality.
  • Kept the details/ComponentStorageBase.hh header with minimal code not to break public API.
• Renamed the new ComponentStorage class to EntityStorage, so it's not confused with the old class.

I considered changing the CreateComponent function to NewComponent, for example, so we could deprecate the old one that returns ComponentKey. But that would mean many systems would need to be updated to use the new function. On the other hand, keeping the function name while changing the return type requires zero changes to our systems because they're all ignoring that return type anyway. So I think it will be easier for most users if we just change the return type.

Also removed some runtime warnings from deprecated functions, the compiler warnings should be enough.

[adlarkin]

The PR title and description mention #865, but I believe that you meant #856?

[chapulina]

The PR title and description mention #865, but I believe that you meant #856?

Ahhh numbers!

[adlarkin]

Keep ComponentKey around, it's still a unique identifier, but we use Entity as the ComponentId

I thought about doing something like this as well, but I believe the issue here is that Entity is of type uint64_t, while ComponentId is of type int. So, is it safe/non-breaking to use Entity in place of ComponentId for the sake of preserving ComponentKey? While it's probably unlikely that we would have any issues casting between the two types, it is worth noting that one type is a signed integer while the other type is unsigned. So, we would need to make sure that we don't have any strange integer overflow bugs if we were to use Entity in place of ComponentId for ComponentKey.

Renamed the new ComponentStorage class to EntityStorage, so it's not confused with the old class.

I'm fine with renaming this class to avoid confusion, but I'm not sure how I feel about EntityStorage. To me, this name makes it sound like we are only storing/managing entities, and not components. But in reality, the driving factor for this class is to provide a way to store components for entities. What if we re-named it to EntityComponentStorage? Would that differ enough from the old class?

[chapulina]

What if we re-named it to EntityComponentStorage?

That works for me 👍 As I mentioned on the other PR, I'm still in the process of understanding what this class is for 😄

is it safe/non-breaking to use Entity in place of ComponentId

Yeah I thought of that too. It's unfortunate that ComponentId is int, because I'm pretty sure it's not meant to store negative values. There's a real possibility of overflow.

Some options are changing ComponentId's type to match Entity's, or change ComponentKey to hold an Entity instead. I think both options should be seamless for users who are being strict about using the aliases. What do you think?

[adlarkin]

As I mentioned on the other PR, I'm still in the process of understanding what this class is for

I left a comment that explains things in greater detail: #856 (comment)

Hopefully that helps!

It's unfortunate that ComponentId is int, because I'm pretty sure it's not meant to store negative values. There's a real possibility of overflow.

I would think that ComponentId is int due to the intention of returning something like -1 as an invalid ID. In fact, it looks like there's an alias that does exactly this: https://github.com/ignitionrobotics/ign-gazebo/blob/1117e5da9b71e6e2f05d7ceef47a6815c91cbec4/include/ignition/gazebo/Types.hh#L99-L100

However, looking at the code for ComponentStorageBase, it looks like it's not being used 🤔: https://github.com/ignitionrobotics/ign-gazebo/blob/97935d96eac49e76bab5c28603eb1db5d77cfb20/include/ignition/gazebo/detail/ComponentStorageBase.hh#L158

Some options are changing ComponentId's type to match Entity's, or change ComponentKey to hold an Entity instead. I think both options should be seamless for users who are being strict about using the aliases. What do you think?

Since either one of these approaches will result in a "breaking" change, I'd prefer changing ComponentKey to hold an Entity so that the code is easier to read/understand. If we're going to make a change, we might as well make the one that produces the clearest results 🤓 But if anyone else has a good reason for changing ComponentId's type to match Entity's type, I'd be interested to hear it.

[chapulina]

What if we re-named it to EntityComponentStorage?

Done 5bb0efc

I'd prefer changing ComponentKey to hold an Entity

Done 5bb0efc

[adlarkin]

Thanks for the suggestions! I think this makes the transition a smoother process.

As we discussed offline, I went ahead and renamed ComponentStorage to EntityComponentStorage in d3e5a25 to keep authorship of those files - so, once you merge in the latest changes from #856 and address the comment I left below, I think we can merge this 👍

All feedback addressed