Add an Array variant to DataPack's Read/Write methods#1219
Add an Array variant to DataPack's Read/Write methods#1219Headline merged 6 commits intoalliedmodders:masterfrom Deathreus:vector-dp
Conversation
asherkin
left a comment
asherkin
left a comment
There was a problem hiding this comment.
While I think the feature is a good idea, I don't think the implementation here is really in spirit with the datapack API which is designed to have some level of type-safety as a counterpoint to the indexed/linear access. This is just packing whatever as an load of cells and relying on the caller to do the right thing.
I think we should have separate CellArray and FloatArray variants (to match the single-element functions, and so we can offer the correct return type back) and the packing should have new internal types added for each which explicitly stores the length of the stored data and the data as a single packed item - thus requiring the caller to match up writing and reading.
|
So actual arrays instead, I see, I'll look into it |
This takes care of the type safety that was inherent with DataPacks, ensuring you can't perform an array operation on a non-array element, and also that you are performing correct type matching, and you aren't going to underflow the buffer I haven't done throrough testing at all, just enough that determines it to be working with Vector arrays
|
@asherkin |
Headline
left a comment
Headline
left a comment
There was a problem hiding this comment.
A bunch of style review and some other comments inline
peace-maker
left a comment
peace-maker
left a comment
There was a problem hiding this comment.
I think it's weird, that the C++ API has the same type signature for the float and non-float variants. I'd expect it to use a float * instead of the same datatype and just using a different internal type flag.
asherkin
left a comment
asherkin
left a comment
There was a problem hiding this comment.
Almost there I think! Couple more inlines from me. As the functions are fairly identical pairs I’ve only left comments on one or the other.
|
Is there anything missing still? |
6 participants
I have not pen-tested this at all and only did a minimal functionality check with this plugin for proof of concept to handling vectors
With the pandemic going on I've been getting all my projects on the back burner out of the way, this being one of them