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

Add an Array variant to DataPack's Read/Write methods #1219

Merged
merged 6 commits into from
May 8, 2020
Merged

Add an Array variant to DataPack's Read/Write methods #1219

merged 6 commits into from
May 8, 2020

Conversation

Deathreus
Copy link
Contributor

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

Copy link
Member

@asherkin asherkin left a comment

Choose a reason for hiding this comment

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

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.

@Deathreus
Copy link
Contributor Author

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
@Deathreus
Copy link
Contributor Author

@asherkin
How does that look?

Copy link
Member

@Headline Headline left a comment

Choose a reason for hiding this comment

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

A bunch of style review and some other comments inline

core/logic/smn_datapacks.cpp Outdated Show resolved Hide resolved
core/logic/smn_datapacks.cpp Outdated Show resolved Hide resolved
core/logic/smn_datapacks.cpp Outdated Show resolved Hide resolved
core/logic/smn_datapacks.cpp Outdated Show resolved Hide resolved
core/logic/smn_datapacks.cpp Outdated Show resolved Hide resolved
core/logic/CDataPack.h Show resolved Hide resolved
core/logic/CDataPack.h Show resolved Hide resolved
core/logic/CDataPack.cpp Outdated Show resolved Hide resolved
core/logic/CDataPack.cpp Outdated Show resolved Hide resolved
core/logic/CDataPack.cpp Outdated Show resolved Hide resolved
@Headline Headline self-requested a review April 10, 2020 02:48
Copy link
Member

@Headline Headline left a comment

Choose a reason for hiding this comment

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

Oops; this looks good to me. I'll let a take a last look at it before the 🚢

@Headline Headline requested a review from asherkin April 10, 2020 02:49
Copy link
Member

@peace-maker peace-maker left a comment

Choose a reason for hiding this comment

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

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.

plugins/include/datapack.inc Outdated Show resolved Hide resolved
Copy link
Member

@asherkin asherkin left a comment

Choose a reason for hiding this comment

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

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.

core/logic/smn_datapacks.cpp Outdated Show resolved Hide resolved
plugins/include/datapack.inc Outdated Show resolved Hide resolved
core/logic/CDataPack.cpp Outdated Show resolved Hide resolved
core/logic/CDataPack.cpp Outdated Show resolved Hide resolved
core/logic/CDataPack.cpp Outdated Show resolved Hide resolved
core/logic/smn_datapacks.cpp Outdated Show resolved Hide resolved
@Deathreus
Copy link
Contributor Author

Is there anything missing still?

@Headline Headline added the Feature Request user requested feature label May 8, 2020
@asherkin asherkin dismissed their stale review May 8, 2020 22:22

Outdated

@Headline Headline merged commit 13621a1 into alliedmodders:master May 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Request user requested feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants