Skip to content
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

Adding support for arbritrary NBT tags #5015

Closed
wants to merge 18 commits into from
Closed

Conversation

12xx12
Copy link
Member

@12xx12 12xx12 commented Oct 31, 2020

see #4179

Added Compound and types
Added Conversion for cParsedNBT and cFastNBT

@12xx12
Copy link
Member Author

12xx12 commented Oct 31, 2020

AAAAAAAAAA - this compiles in MVSC :/

{
using cByteArray = std::vector<char>; // Todo: Make this a Int8
using cIntArray = std::vector<Int32>;
using cList = std::vector<cNBT>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A list isn't just an array of tags, all the entries need to have the same tag type.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cNBT::Push enforces this I believe

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually yes, BUT you can create a cList object and create a cNBT from this and have a list with various types of entries

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i tried to fix this

@12xx12
Copy link
Member Author

12xx12 commented Oct 31, 2020

Hm. Clang doesn't like the circular behaviour.

I'd suggest creating a cCompound class that stores dynamically allocated cNBT objects in a map. Directly using a unordered map works too but then the user has to keep track of the memory leaks. I'd rather use the cCompound to take care of this

@tigerw
Copy link
Member

tigerw commented Oct 31, 2020

What's the issue with circular things? We shouldn't need any sort of inheritance right?

@12xx12
Copy link
Member Author

12xx12 commented Oct 31, 2020

We have compounds including cNBT objects

And we have cNBT objects containing the variant containing compounds

@tigerw
Copy link
Member

tigerw commented Oct 31, 2020

It should just work, I think. I.e. with the example in the other PR this compiles:

    NBT Root{
        NBT::Compound{
            { "a", { NBT::List() } },
            { "b", { 5 } }
        }
    };

@tigerw
Copy link
Member

tigerw commented Oct 31, 2020

Of course it depends on undefined behaviour of std::map accepting incomplete types; so we should probably look to storing a std::unique_ptr in an unordered_map for your solution

@12xx12
Copy link
Member Author

12xx12 commented Oct 31, 2020

ok - i made the variant contain a pointer to cCompund. Now the default constructor of cNBT creates a cCompund with new and deletes it

@12xx12
Copy link
Member Author

12xx12 commented Oct 31, 2020

I think i'm gonna hide some things in the private parts (yeah sorry) of the cNBT class

12xx12 added 2 commits October 31, 2020 22:46
added direct access to compound
serialize takes a cNBT
@12xx12
Copy link
Member Author

12xx12 commented Nov 4, 2020

wrong pr

float, // TAG_Float
double, // TAG_Double
AString, // TAG_String
cCompound *, // TAG_Compound
Copy link
Member Author

@12xx12 12xx12 Nov 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a pointer to cNBT would be better to assure safety even for nested lists

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to use a smart pointer but that blows up :/

@tigerw
Copy link
Member

tigerw commented Mar 16, 2021

There was discussion on your forum thread (https://forum.cuberite.org/thread-3325.html), the main outcome was storing a std::variant containing every thing we need (as objects) into cItem, for better type safety and more ergonomic field accesses. If we do that a cNBT class isn't immediately required anymore.

@12xx12
Copy link
Member Author

12xx12 commented Mar 16, 2021

But how do we access the values in the vector of variants - do we set constant indexes for certain values? Or do we actually map values from string to values?

@tigerw
Copy link
Member

tigerw commented Mar 17, 2021

We can say std::get<PropertyClassHere>(Item.Properties)

@12xx12
Copy link
Member Author

12xx12 commented Mar 17, 2021

Didn't know this works on a vector of variant? What is the behaviour if there are multiple values of the same type?

@tigerw
Copy link
Member

tigerw commented Mar 17, 2021

Oh I see what you mean, we do a linear search and the first variant that matches counts. A map/set is conceptually more appropriate but I'd say is too high overhead for a couple largely fixed values.

@12xx12
Copy link
Member Author

12xx12 commented Mar 17, 2021

Yeah this makes more sense - there won't be that many entries in the vector so that linear search wont be that slow

@12xx12
Copy link
Member Author

12xx12 commented Nov 7, 2022

Closed in favour of #5454

@12xx12 12xx12 closed this Nov 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants