fallahn / tmxlite

lightweight C++14 parser for Tiled tmx files
Other
404 stars 69 forks source link

Partial Tiled 1.8 Support #151

Closed Karamu98 closed 1 year ago

Karamu98 commented 1 year ago

This partially implements this change log. I threw this together pretty quick but I'm happy to make changes if this isn't good enough right now, I'm not a fan of the recursive vectors for example but it got the job done for me!

fallahn commented 1 year ago

Thanks! I'll have a good look ASAP

Karamu98 commented 1 year ago

Is this just a missing include for linux? I can pop that at the top of the file with a problem or is there a better place? Sorry never built for linux before!

fallahn commented 1 year ago

Yeah - looks like it's just missing the #include <memory> - not uncommon for different platforms. In fact I'm surprised it's not missing on mac too (it can be quite fussy). I usually keep a VM handy for this sort of thing, but if you prefer to update and push to your fork the CI should automatically kick in and re-run the checks.

fallahn commented 1 year ago

Looking more closely, I wonder if the shared_ptr is even needed? m_classValue should work just as well as a std::vector<Property> , I think. Perhaps if there is a clear need for there to be only a single instance of a class member then it would make sense (although even then probably fine to use references to a unique_ptr) - but as copying a Property copies all the other possible types it could contain, then I don't see why copying its class type member would be a problem. If you see what I mean?

Karamu98 commented 1 year ago

That's a good point, I reactively threw it onto the heap but that really isn't needed here! I'll get that changed:)

fallahn commented 1 year ago

Nice work - thanks! 😁