Before the patch nodes were added before the current one (selected with axis arrows). Now it will be after the current.
Details
Details
- Reviewers
elexis - Group Reviewers
Restricted Owners Package (Owns No Changed Paths) - Commits
- rP19864: Insert cinematic path nodes after the currently selected node instead of before…
- Trac Tickets
- #3871
- Open Atlas
- Create new/Edit current path and add new nodes to the end by Insert
- Run cinematics
Diff Detail
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Event Timeline
Comment Actions
Build is green
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK! Checking XML files...
http://jw:8080/job/phabricator/1582/ for more details.
Comment Actions
Agree with the change, because the user sets one node, puts it into the correct place, then wants to add another node. But if it's not appended, then the last node position will be changed, i.e. duplicate work for the user.
The patch is complete because it's the only call to InsertNode and both target and position spline use it.
I don't understand why we need the swap for the Spline, can you give me a clue?
| source/maths/NUSpline.cpp | ||
|---|---|---|
| 21 ↗ | (On Diff #2610) | I can't find it in the https://trac.wildfiregames.com/wiki/Coding_Conventions but standard library headers should appear after precompiled and NUSpline.h. Had to look for many files until I found a good example: source/i18n/L10n.cpp |
| 219 ↗ | (On Diff #2610) | This comment is obsolete now, isn't it? |
| 230 ↗ | (On Diff #2610) | (could use initialization list in case we change any of the affected lines) |
Comment Actions
We need it because Distance does mean a distance to the next node. So if we added a node before the next one, we need to restore this distance. I.e.
- Path (1)--5-->(2)
- Add a node before 2 with distance 3
- Path (1)--5-->(2)--3-->(3)
- Swap to fix distance
- Path (1)--3-->(2)--5-->(3)
| source/maths/NUSpline.cpp | ||
|---|---|---|
| 219 ↗ | (On Diff #2610) | No, it's true. |
| 230 ↗ | (On Diff #2610) | We don't have a ctor for initialization list. |
Comment Actions
Ok, so that wasn't me being stupid and somehow creating the entire path in my first danubius path test in reverse, but this code behavior the patch tries to fix.
Reproduce:
- Start atlas
- Show cinematic paths
- Add path
- Select the position or target node and press Insert to add a new node
- Move that new node to the same location as the other node on the x/z plane, but much higher on the y axis
- Save the map
- When we open the XML file, we see that the node with the higher y coordinate appears first, while it was added later. So it's the opposite of the expected order.
That cinematic paths are even played this way can be seen by deleting the path on the Cinema Demo map and creating a new one named "test" that is just supposed to move backwards but move forwards.
So this is a really important patch!
Comment Actions
See 2017-06-06-QuakeNet-#0ad-dev.log for the original bugreport.
Patch is complete, because all places where cinema spline nodes are inserted (one occurence) are handled.
| source/maths/NUSpline.cpp | ||
|---|---|---|
| 222 ↗ | (On Diff #2610) | This must have been a separate bug then. If inserting after a node at NodeCount is possible, then inserting a node before NodeCount must hav ebeen possible before the rest of the patch too. |
| 235 ↗ | (On Diff #2610) | Unfortunately my understanding of this line doesn't improve the more often I look at it. It was discussed on 2017-06-22-QuakeNet-#0ad-dev.log, especially:
But I give in to the fact that the code doesn't work if we don't have it (functionality completely broken in atlas). Might have to go back to the code design sometime. If we add a node with time 1, that time should state how long this node is played, i.e. the temporal distance from the current to the next node. |
| source/tools/atlas/GameInterface/Handlers/CinemaHandler.cpp | ||
| 343 ↗ | (On Diff #2610) | ack |