Add a "speed" property to path nodes.

[divVerent]

To not break the current level format compatibility, this property is editor
only and the "time" property remains authoritative. However, whenever speed
is set, any editor action touching the node will update time according to speed
and the distance to the next node.

[tobbi]

I gotta ask: Why is there a const iterator for next_node but not one for prev_node ?

[divVerent]

Only reason is that I didn't need a const one for prev, but had to query the next one to simplify the existing function to find the arrow vector as that method is const. OTOH I needed a non const iterator too so I can modify it. Can undo that refactor of the arrow function if you want.

…

On Thu, Jun 11, 2020, 17:50 Tobias Markus ***@***.***> wrote: I gotta ask: Why is there a const iterator for next_node but not one for prev_node ? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1424 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAB5NMFAOQZR5AHRPIYKU6DRWFGSXANCNFSM4NZ5FCYA> .

[tobbi]

No, it's perfectly fine :) I was just wondering, that's all.

[divVerent]

Actually, I just noticed that the mutable version of next_node is unused, so I'm gonna delete it. So previous node is only ever used mutably, next node only ever used immutably, mainly because the time for node A to node B is stored in node A.

[Zwatotem]

For me type name std::vector<Path::Node>::iterator seems a bit long and hard to read. It could be my lack of experience in bigger projects. Also I'm not sure, what is our policy regarding typedef.

[tobbi]

We have a couple typedef's lying around the code, so we definitely use them.

[Zwatotem]

I think you should change everything back to mutable, (or maybe that's not the cause) because I'm getting read access violation exception upon deleting nodes.

[divVerent]

I think you should change everything back to mutable, (or maybe that's not the cause) because I'm getting read access violation exception upon deleting nodes.

I cannot reproduce; however I did find an unrelated bug in node deletion. Deleting the first node of a new (size 1) path TWICE crashes:

888	  for (auto i = m_edited_path->m_nodes.begin(); i != m_edited_path->m_nodes.end(); ++i) {
(gdb) print m_edited_path->m_nodes.size()
$9 = -1

Didn't know you can make a std::vector have negative size ;)

Anyway, can you explain how you caused the read access violation by deleting nodes? Like, something of the sort "make new path, add 5 nodes, then delete 3rd, then 4th, then 1st, then 1st"?

[divVerent]

Sent PRs for all crashes I know (all pre-existing), including this one about node deletion I just found.

[Zwatotem]

So as precisely, as possible:
(I was debugging on VS2019.) Open level editor, create any new level (or go to existing one). Add a flying platform object and select path object. Add new nodes to the path. Then change your tool to eraser, and select any of existing nodes.
However I just realized this doesn't occur on Release build.

[divVerent]

I can't reproduce it that very way (but am on Linux/x86_64. Does it matter for you which node you delete?

Also, I am very sure the constness can't be at fault, as one would need a const_cast to cause any runtime problems with that. Can you maybe obtain a backtrace of the failure in VS2019?

[Zwatotem]

No, the choice of deleted node didn't matter.

I can't provide you a backtrace anymore, since VS is making problems with building that I have no time to fix now. However I feel this is not a big problem, since on .msi from appveyor everything works smoothly (Except first node deletion)

Edit: I have couple more notes on how the thing should look like, (e.g. defaulting editor to insert new paths with constant speed, not time), but be the one to decide, if it's in scope of this PR or not.

[divVerent]

I just managed to repro a crash: when the editor is in copy mode, and you use the erase tool on a node marker, then it crashes, at least with ASan enabled.

Of course, copy mode + erase tool itself is nonsense right now, as it makes a copy then erases it immediately - nevertheless this seems worth fixing.

[divVerent]

Wait no - ANY erase of a platform triggers asan. In code unrelated to my change.

[divVerent]

ASan fixed by bfd11ac - the crash that I did find fixed by b9cf4f3. Can you try again on Windows?

[Zwatotem]

Okay, seems woking correctly. Merging.