mapeditor / rs-tiled

Reads files from the Tiled editor into Rust
https://crates.io/crates/tiled
MIT License
268 stars 103 forks source link

Remove `Data` types from the interface #158

Closed aleokdev closed 2 years ago

aleokdev commented 2 years ago

135 forced us to think about some way to replace GIDs with something else that would uniquely identify a tile. References were out of the question since lifetimes would have been a pain to deal with; As such, we ended up on a design which divided types into two categories: Data and MapWrapper<'map, Data>. The first one stores all the raw data related to the object and fully owns it; The second one has a reference to the data and its parent map. This design was chosen because it would allow to have layer functions which relied in map information without having the user pass the map into these functions.

The issue now is that, although this division makes sense internally, user-side it is a mess. Functions that require the map are put in the wrapper, while everything else is in the Data member accessed through .data(). Sometimes the usage of .data() feels somewhat random; it is also inconsistent for some types such as LayerType, which has no data counterpart and is actually not a MapWrapper specialization. As of #157 the interface looks like this: Screenshot_20220212_160822

I suggest to make Data types internal, and replace them in the interface with properties in their respective wrappers. The main disadvantage of this approach is that it adds a lot of boilerplate code to the library (one function per data member) but it has a lot of advantages such as being able to control more precisely what the user gets access to, and having a much simpler interface.

What do you think?

bjorn commented 2 years ago

Thanks for the nice visual overview! Your proposal makes a lot of sense to me. It'd be great if we can get rid of the .data() calls this way.

In C++ it is common practice to keep the internal data structure private and only accessible through member functions, in the interest of keeping binary compatibility. I guess this kind of compatibility is also useful in Rust, maybe even more so because adding members can even break source compatibility.