-
-
Notifications
You must be signed in to change notification settings - Fork 342
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Persistence API #272
Persistence API #272
Conversation
e514a3f
to
d8857df
Compare
👍 |
Nice, I really like this. Bukkit had serialization, but oddly it separated it from NBT, making converting from one to the other difficult. |
This look very nice.
|
Why? That defeats the purpose of telling developers what keys to use for getting certain data.
I don't understand what this would do. |
|
8c63e44
to
e8fd403
Compare
* @param <T> The type of {@link DataSerializable} object | ||
* @return The deserialized object, if available | ||
*/ | ||
<T extends DataSerializable> Optional<T> getSerialiable(String path, Class<T> clazz); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getSerialiable
-> getSerializable
7bf77f5
to
5af3346
Compare
} | ||
|
||
public String getFirst() { | ||
return this.path.substring(this.path.indexOf(this.separator)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will not work if path does not contain any separators.
d14941c
to
c024706
Compare
ff4a504
to
cb5f31d
Compare
Updated the main comment with a quasi todo list. |
/** | ||
* A mutable complete representation of an entity type and its associated data. | ||
* <p>Being that this is a snapshot, all the data from {@link #serializeToContainer()} may |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this is a snapshot, all serialized data is thread-safe and may be stored.
@SpongePowered/developers would like some more input on this PR to get it underway. Currently, we're pondering options on removing the annotations to reduce the "magic voodoo" of annotations. Essentially, we'd be leaving the entire decision making of the tags to the implementation and just state that the data objects translate into DataViews/Containers. |
Hmm, I liked the Annotations, they seemed much nicer and conveyed more information about what's going on. @gabizou can you please show the Banner example without tags for some context? |
I don't think giving the implementation most of the control is a good idea |
I'd love to keep the annotations around, they're not that magic. |
@modwizcode the BannerData would look exactly the same without annotations: public interface BannerData extends DataSerializable {
String getId();
Vec3i getPosition();
DyeColor getBaseColor();
void setBaseColor(DyeColor color);
List<BannerPattern> getPatterns();
void addPattern(BannerPattern pattern);
void setPatterns(List<BannerPatterns> patterns);
interface BannerPattern extends DataSerializable {
DyeColor getColor();
List<String> getPatterns();
}
} |
They're somewhat magic when it comes to debugging why something is breaking in the serialization process because the annotation doesn't exist. Plus it makes writing future parts of the API a bit convoluted and dependent on the Minecraft implementation. |
the annotations do have some uses when the serialized fields need to have different names that can't be java method names |
@progwml6 I think the idea is that the implementation would define the names, not the methods. This itself is a bad idea, because now we can't have custom classes. |
the annotations need to at a minimum alert the implementation that a field/method requires a specific name in order to get some information from a forge mod, etc. |
While I may sound like a broken record, I like how Jackson does it: http://wiki.fasterxml.com/JacksonAnnotations
Jackson is a pretty powerful and big package, but those are the features most relevant to us in my opinion. The polymorphic type handling would be nice, but whether it's worth the effort in our specific case is another question (although it is pretty handy when working with real use cases). In addition, Jackson lets you configure many of those things without needing to actually annotate the class. |
Just my opinion: Pros of using Jackson annotations
Cons:
|
fbadec7
to
31f7666
Compare
…r, DataView, and DataOptions. Mark potential candidates for DataSerializable. Add DataPath for annotating keys to fetch relative data in DataSerializables.
…ects in DataView. Clean up MemoryDataView and implement remaining getList methods.
Remove unecessary override in ChunkIterator.
Remove persistence annotations pending future PR.
31f7666
to
e458793
Compare
Persistence API introduces the common data structure to represent any possible data produced by the SpongeAPI.
This adds interfaces for
DataContainer
,DataView
, andDataOptions
.Using a
DataContainer
is very simple:Being that these data structures are map based, serialization to various data handlers, such as NBT, flat file, SQL, etc. is possible.
As well, adding annotations for
SerializableAs
andDataPath
to mark methods and fields with keys that can easily be referenced for automated serialization (example: Gson).Example usage:
Finally, displaying the importance of the viability for
BannerData
as aDataContainer
, the following would exist:As visible in the example, the map based data structure,
DataContainer#getString("id")
would return the actual string id provided from theBannerData
and likewise,DyeColor color = BannerData.serialize().getList("patterns").get(0).getSerializable("color", DyeColor.class);
In essence, we can now safely use these maps to not only serialize and deserialize SpongeAPI objects, but internally, we can use various implementations to not just translate, but manipulate similar data structures in NBT (like in this case, banner data).
Likewise, all of the
DataContainer
system is designed to allow serializingEntitySnapshots
for various reasons.ToDo: