Implementing banner

[xoft]

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.

[Seyaku]

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

[xoft]

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.

[Seyaku]

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);
        }
    }

[Seyaku]

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.

[xoft]

I don't think there is a way as of now, you'll need to come up with something.

[sphinxc0re]

@Seyaku actually there has to be a way because the crafting recipe for cake is working correctly and it doesn't consume the bucket

[Seyaku]

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.

[tigerw]

I would advise against storing anything as JSON in memory, considering saves are in NBT format.

[Seyaku]

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.