lua: crash when setting two actions to same position and time

[SleepyPrince]

If 2 actions are set to be at the same position and time, OFS will crash.

function test()
    script.actions[1].at = script.actions[2].at
    script.actions[1].pos = script.actions[2].pos
    ofs.Commit(script)
end

OFS should delete the duplicate point at commit instead of crashing

[OpenFunscripter]

I agree it shouldn't crash. But the solution is not that simple.

I implemented it so it would delete the duplicate point however this causes this very unintuitive behavior below.

function test2()
    local script = ofs.Script(ofs.ActiveIdx())
        -- this line causes the first action to be deleted
    script.actions[1].at = script.actions[2].at
        -- since the first action was deleted this now sets
        -- what was previously the 'second' actions position to 
        -- what was previously the 'third' actions position
    script.actions[1].pos = script.actions[2].pos
    ofs.Commit(script)
end

I hope this is understandable 😅

Anyway I think the behaviour above can also be seen as a bug (eventhough it makes sense when you understand whats going on). Because of that I would say that the code above should crash the Lua extension but not OFS.

[SleepyPrince]

I understand the behavior you described above, but it would be a better approach to only do the duplicate removal (and other audits) at the time of commit, as currently accidentally setting 2 actions to the same time would cause all indexes to shift. The index shifts also happens with RemoveAction. I believe it would be better to get a static copy of all actions at the start of script, do all the modification (including marking actions to be deleted) then use ofs.Commit to really audit and commit the changes, since no action is going to be applied if you don't call ofs.Commit.

edit: just did another test and it seems the behavior is not as I expected

function test()
    script.actions[1].pos = 10
    script.actions[2].pos = 20
    script.actions[1].at = script.actions[2].at
    ofs.Commit(script)
end

This would cause 2 actions to be at the same time without deletion

[OpenFunscripter]

I believe it would be better to get a static copy of all actions at the start of script, do all the modification (including marking actions to be deleted) then use ofs.Commit to really audit and commit the changes, since no action is going to be applied if you don't call ofs.Commit.

I'm not opposed to that. It would be a bit of a rewrite with potential breaking changes to existing extensions. However this change would have the potential to make things cleaner.

Full transparency: I sadly don't have any time to do anything substantial on OFS right now. Hopefully in 2 months from now I'll have time for such things again but right now I have alot of stuff going on. Until then keep this issue open so I don't forget.

[SleepyPrince]

Appreciate your work on creating OFS, take your time.

[OpenFunscripter]

I have started a rewrite of the Lua API.

Right now I'm planning to make it so that the actions array is just a regular dynamic array instead of an ordered array. Meaning you can insert actions out of order as well as duplicates without any problems. It wouldn't implicitly reorder the actions. On commit they are then chronologically ordered and duplicates would be silently ignored (or an error could returned. not sure what's better).

There would be a a new function script:sort() which would sort the actions array. If someone inserts actions at the end and then wants iterate chronologically afterwards they would call script:sort() first.

The marking actions for deletion I would add as two seperate functions rather than doing it on commit.

function do_something()
  local script = ofs.Script(ofs.ActiveIdx())
  for idx, a in ipairs(script.actions) do
     if a.pos > 50 then
       script:markForRemoval(idx) -- marks the action at the index for removal
     end
  end
  script:removeMarked() -- actually removes all the marked actions
  script:commit()
end

As you can see I'm making it a little more object oriented by making some functions member functions which in Lua are invoked with the colon operator.

[SleepyPrince]

This sounds like a good approach to me. For the commit part, is it possible to have the function return the error (not sure in what form) to a variable like "err = script:commit()" So it is up to the scripter to decide how to handle the error?

[OpenFunscripter]

I don't like that to be honest. The only malformed commit I can imagine is duplicate timestamps and don't see what is there to handle. It would just be an error in the extension as far as I'm concerned and I would either silently ignore the duplicates or halt the extension and display an error.

[SleepyPrince]

You are right, we should focus on making the actions right before the commit. I would prefer it to throw error, as silently ignore may cause ambiguity to which one to keep (e.g. what will sort() do if two actions have the same time)

[OpenFunscripter]

I can definitely throw an error if there happen to be duplicate timestamps.

what will sort() do if two actions have the same time

Good question I have no idea. Currently I'm just using C++ STL std::sort.

TIL: Apparently there's a std::stable_sort I can use for exactly this case which preserves the insertion order for equal elements. https://en.cppreference.com/w/cpp/algorithm/stable_sort

Sorts the elements in the range [first, last) in non-descending order. The order of equivalent elements is guaranteed to be preserved.

The more you know. 🌠

[SleepyPrince]

If an error will be thrown when there is duplicate timestamps, then sorting won't really help as we still need to fix the duplicate before commit.

How about adding an extra parameter can be added to ClosestAction, i.e. ofs.ClosestAction(script, time, delta) If a delta is provided, it will return the closest action if the absolute time difference is <= delta. If no delta is provided, it will return the closest action irrespective of how far they are apart.

so that If I want to add a new action, I can check if there is an existing action first, then I can either edit the original action directly or I can remove it and add a new one.

[OpenFunscripter]

How about adding an extra parameter can be added to ClosestAction, i.e. ofs.ClosestAction(script, time, delta) If a delta is provided, it will return the closest action if the absolute time difference is <= delta. If no delta is provided, it will return the closest action irrespective of how far they are apart.

You can implement this trivially in Lua so I'm kinda against that as well. I want to keep the API as lean as possible, a set of building blocks with which you can do everything you'd want to be able to do. It would defeat the purpose if everything was already a prebuilt function.

function closestAction(script, time, delta)
  local action, idx = script:closestAction(time)
  if math.abs(action.at - time) <= delta then
    return action, idx
  else
    return nil
  end
end

[SleepyPrince]

understandable, looking forward to the new implementation

[OpenFunscripter]

Yes! It's all still on a local branch about 70% done. But no etas it will be done when it's done. 😬

[OpenFunscripter]

There's now a new release