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

Lua Writable Collections #160

Merged
merged 11 commits into from
Dec 7, 2024
Merged

Lua Writable Collections #160

merged 11 commits into from
Dec 7, 2024

Conversation

Bit-Crust
Copy link
Contributor

A number of properties which were exposed to Lua as iterators could not have their elements modified. When an iterator was obtained for Lua, it returned an iterator over a list of copies of elements from the original C++ collection, meaning that changes to those copies would be ineffective. Affected collections included MOSR Gibs, AEmitter or PEmitter Emissions, and MetaMan Players, unaffected were all collections of pointers. Corrected by changing all of those lists to pointers.

Bit-Crust and others added 6 commits November 15, 2024 15:11
… be written to without access to C++ data structure construction methods for various types) by making them read only. Made AEmitter and PEmitter Emissions individually Lua writable, added getter and setter for Emission accumulator.
…ua by changing referenced std iterators to work over pointers instead of raw values, meaning the value copies passed to lua are value copies of pointers, not raw values.
@Causeless
Copy link
Contributor

Causeless commented Nov 20, 2024

Oof, I can understand this but it feels really awkward doing it like this. It has performance implications, it messies the code, and it introduces a lot of manual memory management.

This is probably the safest approach, but at the same time it feels really awkward to have to do this everywhere we want to expose things to Lua...

Some thoughts:

  1. We could instead have an intermediate lua binding that constructs a list of pointers to the Gib in the vector, instead of intrinisically actually allocating it seperately. This would avoid manual memory management and would also be more performant for the main C++ sim stuff as it'd avoid pointer chasing and potential cache misses there. However, if the vectors are modified, the elements within would become invalidated and bring the risk of Lua stamping over the wrong memory. To be fair this is already an issue that scripters are somewhat accustomed to dealing with though, as MO references in Lua aren't guaranteed to be stable for any longer than a frame.
  2. We could do what we're doing here, but use unique ptrs instead of raw ptrs. Would be basically the same and still a bit awkward, but at least we get to avoid manual memory management.
  3. We could have some entirely object that represents an entity id + index in the list and dereferences back onto the real object, while seeming just like a reference on the lua side. This would still be potentially risky but would be much less so

Interested in your thoughts

@Causeless
Copy link
Contributor

Think it's still worth getting this in, so merging

@Causeless Causeless enabled auto-merge December 7, 2024 13:19
@Bit-Crust Bit-Crust disabled auto-merge December 7, 2024 13:21
@Bit-Crust
Copy link
Contributor Author

I disabled automerge for the moment, could've sworn I had an outgoing commit to push first, but false alarm. I can't turn it back on, however, presumably until the checks pass.

@Causeless Causeless added this pull request to the merge queue Dec 7, 2024
@Causeless Causeless removed this pull request from the merge queue due to a manual request Dec 7, 2024
@Causeless Causeless added this pull request to the merge queue Dec 7, 2024
Merged via the queue into development with commit bbf1894 Dec 7, 2024
4 checks passed
@Causeless Causeless deleted the lua-writable-collections branch December 7, 2024 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants