+- Cuberite Forum (https://forum.cuberite.org)
+-- Forum: Cuberite (https://forum.cuberite.org/forum-4.html)
+--- Forum: Development (https://forum.cuberite.org/forum-13.html)
+--- Thread: Implementing banner (/thread-2451.html)
RE: Implementing banner - xoft - 05-23-2016
I think you approach has a problem with duplicating a cItem. Why not simply use the Json::Value as the actual storage in cItem? That way you save two translations (FromJson and ToJson) and automatically gain easy duplicating, while still retaining universal access.
RE: Implementing banner - Seyaku - 05-23-2016
Duplication should work just fine and ToNBT is called way more often than ToJson, so not much to win there. But yeah, using json::value to store it could work.
If we change it to Json::Value, we need to add ToNBT and FromNBT to the cItemHandler.
So a quick pro/con for using Json::Value would be:
Pros:
-easier to get to the data (no casting or including)
-easier to expose to lua?
-no need for a custom copy constructor and assignment op in cItem
Cons:
-adds more overhead (slower, more memory use)
-data structure is unclear
-no type checking
RE: Implementing banner - xoft - 05-23-2016
I must be blind - I don't see any duplication code, and cItem uses a unique_ptr, which cannot be copied.
More pros:
- direct correspondence with parsed commands (In the future if we add the same processing as vanilla does for commands, such as "/give ... {detaildata}" )
- future-proof for additional data
As for your cons, I don't think they are that much of a problem. cItem handling is definitely not a bottleneck, so if it uses a bit more CPU, I don't care. Data structure is unclear, which might be a pro as well, since we can add more data without any trouble for other items and for future versions.
RE: Implementing banner - Seyaku - 05-23-2016
ok, I'll rework it with Json::Value, shouldn't be too much work.And look at the copy constructor and assignment op ->
Code:
if (a_CopyFrom.GetItemMeta()) {
cItemMeta * meta = GetHandler()->MakeItemMeta();
if (meta) {
meta->FromCopy(a_CopyFrom.GetItemMeta());
SetItemMeta(meta);
}
}RE: Implementing banner - Seyaku - 05-24-2016
I reworked the code to use Json::Value as m_Metadata, everything works as intended. Currently implementing the crafting side and was wondering how I can prevent a certain ingredient to be consumed during crafting.
RE: Implementing banner - xoft - 05-24-2016
I don't think there is a way as of now, you'll need to come up with something.
RE: Implementing banner - sphinxc0re - 05-24-2016
@Seyaku actually there has to be a way because the crafting recipe for cake is working correctly and it doesn't consume the bucket
RE: Implementing banner - Seyaku - 05-24-2016
Would removing the ingredient from a_Recipe->m_Ingredients work?
---
Everything works as far as I can tell (apart from retaining one item in one recipe), but there is a weird issue when crafting banners. I have to click on the result for it to become visible for some reason.
Tomorrow I will check all the recipes to make sure they all work.
RE: Implementing banner - tigerw - 05-24-2016
I would advise against storing anything as JSON in memory, considering saves are in NBT format.
RE: Implementing banner - Seyaku - 05-24-2016
(05-24-2016, 08:00 AM)tigerw Wrote: I would advise against storing anything as JSON in memory, considering saves are in NBT format.Well I didn't until xoft convinced me it would be better if I did. As for the NBT saving, that part is covered. Might I suggest a vote then? I can see the merit of both approaches, but would like to have consensus on this matter before I make any pull requests.