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

RunPlan / RunPlanVectory operator+ and operator+= #679

Open
ptheywood opened this issue Sep 2, 2021 · 0 comments
Open

RunPlan / RunPlanVectory operator+ and operator+= #679

ptheywood opened this issue Sep 2, 2021 · 0 comments

Comments

@ptheywood
Copy link
Member

ptheywood commented Sep 2, 2021

Currently RunPlan and RunPlanVector have several operator+ and operator+= operator overloads, which combine one or more plans into a single RunPlanVector.

I.e. (for illustration only)

flamegpu::RunPlanVector AB(model, 2); // Contains 2 plans, [A, B]
flamegpu::RunPlanVector CD(model, 2); // Contains 2 plans, [C, D]
flamegpu::RunPlan E(model); // A single Plan, E

// vector + vector
flamegpu::RunPlan ABCD = AB + CD; // contains [A, B, C, D]

// Vector += vector
flamegpu::RunPlan ABCD(model, 0); 
ABCD += AB;
ABCD += CD; // Contains [A, B, C, D]

// Vector + Plan
flamegpu::RunPlanVector ABE = AB + E; // Contains [A, B, E]

// Vector += Plan
flamegpu::RunPlanVector ABE(model, 0);
ABE += AB;
ABE += E; // Contains [A, B, E]

However, as a vector is used internally, this is implemented as push_back only, which leads to potentially unexpected orders when operator+

I.e. (for illustration only)

flamegpu::RunPlanVector AB(model, 2); // Contains 2 plans, [A, B]
flamegpu::RunPlanVector CD(model, 2); // Contains 2 plans, [C, D]
flamegpu::RunPlan E(model); // A single Plan, E

// vector + vector
flamegpu::RunPlan CDAB = CD + AB; // contains [A, B, C, D] - i.e. it was an append not a prepend

// Vector += vector
flamegpu::RunPlan CDAB(model, 0); 
CDAB += AB;
CDAB += CD; // Contains [A, B, C, D], not the expected/intended  [C, D, A, B]

// Vector + Plan
flamegpu::RunPlanVector EAB = E + AB; // Contains [A, B, E] - not the intended/expected [E, A, B]

// Vector += Plan
flamegpu::RunPlanVector EAB(model, 0);
ABE += E;
ABE += AB; // Contains [A, B, E] as expected. Cannot directly E+= AB would however not make sense / be possible.

We could define/implement operator+(lhs, rhs) outside of the classes (and/or friend it in) to enable append / prepend behaviour, but this doesn't make sense for operator+=, nor versions which return a reference like the current implementations.

Given this is essentially a thin wrapper around std::vector, instead we could just implement push_back rather than provide these operator overloads. This might not be as user-friendly, but also avoids operator+ leading to confusion of order.

Or as another alternative, we could move to an underlying implementation other than Vector, which supports high performance prepend/push_front operations (std::list?). There might be other consequences of this, but I think this would still suit the same purpose, and grand-scheme the performance of run plan construction will be negligible compared to the simulations themselves.

Any of these will be a breaking change, so sooner rather than later is better.

These are also not being wrapped correctly by swig currently, so this should be addressed at the same time.

@ptheywood ptheywood changed the title RunPlan / RunPlanVectory operator+ RunPlan / RunPlanVectory operator+ and operator+= Sep 2, 2021
@Robadob Robadob added the bug label Jul 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants