Open forgetmenot13579 opened 11 months ago
Just to clarify, I'll be happy to contribute a PR implementing this functionality, I just wanted to have a discussion on what the API should look like before doing so.
That is indeed a pretty big change, but I believe it to be a good one. In my experience having to hold onto the provider passed in the constructor also seems to be a bit confusing for new users; passing the provider in the tick method may be more intuitive.
This will make it harder to use the auto-sync API though, as now setters will need to take the provider as argument, unless we overhaul that API as well.
P.S. you wrote RegistryKey
and RegistryType
in a few places, I believe that should be ComponentKey
and ComponentType
?
Why would the auto sync setters need to take the provider as an argument? Shouldn't the syncing be based entirely on the data in the buffer, not doing any kind of outside logic?
P.S. you wrote RegistryKey and RegistryType in a few places, I believe that should be ComponentKey and ComponentType ?
Ah, yep, thanks for catching that. Fixed.
Why would the auto sync setters need to take the provider as an argument?
Not the writers, the components' own setters. It's a common pattern that calling e.g. component.setMana(int)
triggers a sync using the held provider, which would not exist anymore.
As for the actual decisions :
ComponentKey
means slightly fewer changes, but ComponentType
would probably be more in line with Minecraft's own registrable object types.isValid
method in Component to have a way to tell that an instance is void ?SelfSerializingComponent
instead of LegacyComponent
, unless you want to emphasise that its use is discouraged in new code.LegacyComponent
was just a stand-in name, I think SelfSerializingComponent
is a fine actual name.
Regarding volatility of components, we could enforce implementation of CopyableComponent
to avoid that problem. Partial syncing changes will also make that less of a problem.
Actually, serialization methods may need a World access as well, for access to e.g. dynamic registries. How would that work with codecs ?
Vanilla's solution is to hold onto RegistryKey
s or RegistryEntryList
s and only resolve them when the value is actually needed, i.e. the tick
method where the context is known and the holder passed, or any logic outside of the component class that accesses the component and therefore would have the holder (and presumably the world) for context. There's a good amount of scaffolding in Vanilla for this pattern as well.
At the moment, (de)serialization is done using
Component#writeToNbt
andComponent#readFromNbt
, which requires users to write out their (de)serialization logic explicitly. This is not only an arduous process, but also introduces a large potential for error.Let's look at a simple example of serialization logic.
The (de)serialization code for this class with the current scheme would look like this:
There are a few very obvious things that can go wrong with manual serialization logic like this:
And these are just for the simple example listed above. There are much more complicated component implementations out there. So what does the alternative look like, in the world of codecs?
This can look like a lot of nonsense to somebody who hasn't worked with codecs, but I won't go into too much detail with how creating codecs works here. What's important to know is that each field of our component is mapped and can be safely serialized and deserialized. We have created our (de)serialization logic in such a way that we can be sure each field will be correctly written to and read from.
Implications
With the
Component#writeToNbt
andComponent#readFromNbt
methods being ingrained so deeply into CCA's codebase and API surface, there's no reasonable way to add codec support without a major version bump. We could hack it and have aCodecComponent
whose serialization methods just threw exceptions if called, but I would not consider that an elegant solution. Instead, we should implement codec components in a way that makes sense and accept that people will have to put in some work to adapt their existing projects.The first thing to do is remove
Component#writeToNbt
andComponent#readFromNbt
from the baseComponent
class. Obviously allowing mods to keep their existing serialization logic is going to be important, so instead these methods will move to a newLegacyComponent
class (name tbd). The baseComponent
class will be just a marker interface from then on.There are essentially two options for associating codecs and components. The first would be to add a
Component#codec
method and get the codec from existing (or default) instances of the component. Due to the limitations of Java's generic system (namely no self typing) this would be a bit ugly. My preferred implementation would be to store the codec on each componentsRegistryKey
, and rename that class toRegistryType
to be more representative of this change in behavior. We would then add two methods<C extends Component> C RegistryType#readFromNbt(ComponentContainer, NbtCompound)
andvoid writeToNbt(ComponentContainer, NbtCompound)
, which would take the place in the API of the existing (de)serialization methods (and be responsible for calling those methods onLegacyComponent
s).Because
Component
is now a marker interface, we need to do some work to ensure that people's components are serializable. Thus, we would replace the current registration methodComponentRegistry#getOrCreate
With two methods
Another change that would be necessary is to the ticking logic. Currently, CCA's tick methods do not take parameters, as components are expected to keep a reference to their provider around if they want to access it. There's not really a good way to pass the provider to a component created with codecs during initialization, so I'd propose that the ticking components are given a generic for their provider type, and that new
tick
,clientTick
, andserverTick
methods are created that are passed that type as an argument.163