- Added the moving tool
- Possibility to add/delete selected nodes
Probably, the way of the tool render isn't clear.
Details
Details
- Reviewers
• leper - Group Reviewers
Restricted Owners Package (Owns No Changed Paths) - Commits
- rP19483: Cinematic path node moving tool for Atlas. Patch by Vladislav.
- Trac Tickets
- #3871
- Run Atlas
- Create a path
- Add a node (select some and press Insert
- Delete a node (select some and press Delete
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
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
Build is green
Updating workspaces.
Build (release)...
In file included from ../../../source/tools/atlas/GameInterface/Handlers/CinemaHandler.cpp:20:0:
../../../source/tools/atlas/GameInterface/Handlers/MessageHandler.h:39:23: warning: unused parameter ‘msg’ [-Wunused-parameter]
void f##t(prefix##t* msg)
^
../../../source/tools/atlas/GameInterface/Handlers/MessageHandler.h:41:27: note: in expansion of macro ‘THINGHANDLER’
#define MESSAGEHANDLER(t) THINGHANDLER(m, Message, t)
^
../../../source/tools/atlas/GameInterface/Handlers/CinemaHandler.cpp:624:1: note: in expansion of macro ‘MESSAGEHANDLER’
MESSAGEHANDLER(ClearPathNodePreview)
^
Build (debug)...
In file included from ../../../source/tools/atlas/GameInterface/Handlers/CinemaHandler.cpp:20:0:
../../../source/tools/atlas/GameInterface/Handlers/MessageHandler.h:39:23: warning: unused parameter ‘msg’ [-Wunused-parameter]
void f##t(prefix##t* msg)
^
../../../source/tools/atlas/GameInterface/Handlers/MessageHandler.h:41:27: note: in expansion of macro ‘THINGHANDLER’
#define MESSAGEHANDLER(t) THINGHANDLER(m, Message, t)
^
../../../source/tools/atlas/GameInterface/Handlers/CinemaHandler.cpp:624:1: note: in expansion of macro ‘MESSAGEHANDLER’
MESSAGEHANDLER(ClearPathNodePreview)
^
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!http://jw:8080/job/phabricator/851/ for more details.
Comment Actions
Build is green
Updating workspaces.
Build (release)...
In file included from ../../../source/tools/atlas/GameInterface/Handlers/CinemaHandler.cpp:20:0:
../../../source/tools/atlas/GameInterface/Handlers/MessageHandler.h:39:23: warning: unused parameter ‘msg’ [-Wunused-parameter]
void f##t(prefix##t* msg)
^
../../../source/tools/atlas/GameInterface/Handlers/MessageHandler.h:41:27: note: in expansion of macro ‘THINGHANDLER’
#define MESSAGEHANDLER(t) THINGHANDLER(m, Message, t)
^
../../../source/tools/atlas/GameInterface/Handlers/CinemaHandler.cpp:624:1: note: in expansion of macro ‘MESSAGEHANDLER’
MESSAGEHANDLER(ClearPathNodePreview)
^
Build (debug)...
In file included from ../../../source/tools/atlas/GameInterface/Handlers/CinemaHandler.cpp:20:0:
../../../source/tools/atlas/GameInterface/Handlers/MessageHandler.h:39:23: warning: unused parameter ‘msg’ [-Wunused-parameter]
void f##t(prefix##t* msg)
^
../../../source/tools/atlas/GameInterface/Handlers/MessageHandler.h:41:27: note: in expansion of macro ‘THINGHANDLER’
#define MESSAGEHANDLER(t) THINGHANDLER(m, Message, t)
^
../../../source/tools/atlas/GameInterface/Handlers/CinemaHandler.cpp:624:1: note: in expansion of macro ‘MESSAGEHANDLER’
MESSAGEHANDLER(ClearPathNodePreview)
^
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!http://jw:8080/job/phabricator/852/ for more details.
| source/tools/atlas/GameInterface/Handlers/CinemaHandler.cpp | ||
|---|---|---|
| 521 ↗ | (On Diff #1418) | And since paths is only used once you could just inline it. |
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!
http://jw:8080/job/phabricator/863/ for more details.
| source/tools/atlas/GameInterface/Handlers/CinemaHandler.cpp | ||
|---|---|---|
| 521 ↗ | (On Diff #1418) | Won't be the line too big? |
| source/tools/atlas/GameInterface/Handlers/CinemaHandler.cpp | ||
|---|---|---|
| 537 ↗ | (On Diff #1430) | This loop is nearly identical to the one above, this slightly makes me wonder if we couldn't unify that (but I guess that would need a template solution, or a macro) and it isn't that bad, but still. |
| 521 ↗ | (On Diff #1418) | It is 4 characters longer than the line with the assignment, and kills that line, so no it wouldn't be. |
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!
http://jw:8080/job/phabricator/878/ for more details.
| source/tools/atlas/GameInterface/Handlers/CinemaHandler.cpp | ||
|---|---|---|
| 287 ↗ | (On Diff #1451) | Could be static. Don't we pass Vector2/3D as const refs elsewhere? |
| 383 ↗ | (On Diff #1451) | This whole thing above is used in most of these handlers, can we unify this? |
| 537 ↗ | (On Diff #1451) | Ah, so CCinemaPath is convertible to TNSpline? Seems even nicer :-) |
| source/tools/atlas/GameInterface/Handlers/CinemaHandler.cpp | ||
|---|---|---|
| 383 ↗ | (On Diff #1451) | That would be an option, but I was talking about the whole function up to this point (or maybe even including the next one, but then we aren't talking about validation only. Could very well be not that nice to actually implement or yield great results, but this sanity checking code does seem duplicated. |
| source/tools/atlas/GameInterface/Handlers/CinemaHandler.cpp | ||
|---|---|---|
| 383 ↗ | (On Diff #1451) | But an intersection of these checks has only node and cmpCinemaManager checks. |
Comment Actions
@leper I heard you are interested in C++ code and you've already read through the patch, do you want to complete the review?
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!
http://jw:8080/job/phabricator/884/ for more details.
Comment Actions
People telling stories again, huh? Someone told me you'd like to give D213 a go in return :P
| source/tools/atlas/GameInterface/Handlers/CinemaHandler.cpp | ||
|---|---|---|
| 581 ↗ | (On Diff #1467) | ENSURE(msg); would be a nicer way to mute the unused warning |
Comment Actions
Some more comments (some already mentioned before).
Tool works, though it takes some getting used to and could get some improvements. We can do those after getting initial support merged though.
Some general comments (these are for later, as opposed to the inline comments):
- Adding new nodes that are below ground level is a bit confusing (took me some time to figure out that they were below ground, might be a good idea using a different colour in that case).
- I would have expected to be able to move the node the x/z plane by default without having to adjust each of them individually. I guess switching between x/z and y (up/down) movement as opposed to all three individually would make this much more user friendly.
- I guess this doesn't really consider target nodes yet (I managed to mess up those slightly, but adding more might be a bit tricky)
- Being able to preview paths right within atlas would be great!
- There is some issue (most likely completely unrelated to the cinematics work) where the visibility isn't reset after ending playing the simulation in atlas.
Nice work so far!
| source/graphics/CinemaManager.cpp | ||
|---|---|---|
| 117 ↗ | (On Diff #1467) | Yes, having both 1.0f and 0.f on the same line is inconsistent, I'd have gone with 1.f and 0.f though, see also the line above. |
| 122 ↗ | (On Diff #1467) | Same 1.f comment as above. |
| source/tools/atlas/AtlasUI/ScenarioEditor/Tools/TransformPath.cpp | ||
| 26 ↗ | (On Diff #1467) | \n before the wx includes. |
| source/tools/atlas/GameInterface/Handlers/CinemaHandler.cpp | ||
| 287 ↗ | (On Diff #1451) | const refs, static. |
| 42 ↗ | (On Diff #1467) | Could be 5.f. |
| 306 ↗ | (On Diff #1467) | static, reference. |
| 528 ↗ | (On Diff #1467) | It's not an iterator, so naming it it is just lying, name it p. |
| 542 ↗ | (On Diff #1467) | static, const refs. |
| 556 ↗ | (On Diff #1467) | This line could be moved somewhere closer to where it is used (and thus after the early return). |
| 576 ↗ | (On Diff #1467) | It might be nice to allow moving in x/y (or x/z in our engine) direction at the same time, only needing some switching for up/down. |
| 581 ↗ | (On Diff #1467) | Or UNUSED2(msg). Because if we'd ENSURE(msg) here, we should do the same thing in all oher handlers in here. |
| source/tools/atlas/GameInterface/SharedTypes.h | ||
| 118 ↗ | (On Diff #1467) | Unrelated to the diff at hand, but why is there commented out code here? |
| 126 ↗ | (On Diff #1467) | All of these members should be Shareable<T>. |
| source/tools/atlas/GameInterface/View.cpp | ||
| 233 ↗ | (On Diff #1467) | Shouldn't this one have an #if CONFIG_GLES block too? |
Comment Actions
I used shifted value + 1.0f, but elexis said to don't increase it, so I made it visible even it's on ground, also it has lines between a spline and ground.
- I would have expected to be able to move the node the x/z plane by default without having to adjust each of them individually. I guess switching between x/z and y (up/down) movement as opposed to all three individually would make this much more user friendly.
I've planned this feature.
- I guess this doesn't really consider target nodes yet (I managed to mess up those slightly, but adding more might be a bit tricky)
What do you mean? You could control target nodes as usual position nodes.
- Being able to preview paths right within atlas would be great!
Yeah, I have already it in some drafts.
- There is some issue (most likely completely unrelated to the cinematics work) where the visibility isn't reset after ending playing the simulation in atlas.
Yep, I've noticed it too, it looks like more atlas issues, but need to look at it deeper.
Comment Actions
Yes, not moving it a lot, but at least using a different colour for the lines below ground would be nice, otherwise someone new to this might get confused. But as said not exactly an urgent issue, but fixing this at some point in the future would be nice.
- I guess this doesn't really consider target nodes yet (I managed to mess up those slightly, but adding more might be a bit tricky)
What do you mean? You could control target nodes as usual position nodes.
Ah, I did play with those right at the start and might just have messed things up due to only figuring out the controls a bit later
- There is some issue (most likely completely unrelated to the cinematics work) where the visibility isn't reset after ending playing the simulation in atlas.
Yep, I've noticed it too, it looks like more atlas issues, but need to look at it deeper.
Care to create a ticket?
| source/tools/atlas/GameInterface/SharedTypes.h | ||
|---|---|---|
| 126 ↗ | (On Diff #1467) | Because the members of something shareable need to be shareable, otherwise calling it that is a blatant lie. If we don't write anything there that still doesn't prevent mismatching implementations from accessing chunks of memory that they shouldn't access. |
| source/tools/atlas/GameInterface/SharedTypes.h | ||
|---|---|---|
| 126 ↗ | (On Diff #1467) | Ok, no problem. If we're talking about really random chunks of memory we always could break it even with Shareable. |
| source/tools/atlas/GameInterface/Handlers/CinemaHandler.cpp | ||
|---|---|---|
| 576 ↗ | (On Diff #1467) | I've planned it, so I used 1, 2, 4, not 1, 2, 3, because then I'll use it as mask. |
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!
http://jw:8080/job/phabricator/892/ for more details.
| source/tools/atlas/AtlasUI/ScenarioEditor/Tools/TransformPath.cpp | ||
|---|---|---|
| 30 ↗ | (On Diff #1490) | Could sort these alphabetically. |
| source/tools/atlas/GameInterface/Handlers/CinemaHandler.cpp | ||
| 576 ↗ | (On Diff #1467) | I did suspect something like that, maybe using defines or enums once that happens could help with readability. |
| 336 ↗ | (On Diff #1490) | Actually since we are duplicating at least path, cmpCinemaManager, and name in all these cases I guess going with a define could be cleaner... Sorry for this back-and-forth, but sometimes you have to see the code first. |
| 490 ↗ | (On Diff #1490) | static |
| 518 ↗ | (On Diff #1490) | Should this be AtlasMessage::sCinemaPathNode node()? Else there isn't any initialization done and assigning that might not result in the expected thing. |
| source/tools/atlas/GameInterface/Handlers/CinemaHandler.cpp | ||
|---|---|---|
| 336 ↗ | (On Diff #1490) | Do want to hide the cmpCinemaManager check? |
| 518 ↗ | (On Diff #1490) | Why? It's classic C++ c-tor, but a compiler reads the 'type name(); string as the function declaration, so AtlasMessage::sCinemaPathNode node()` will get a compilation error. |
| source/tools/atlas/GameInterface/Handlers/CinemaHandler.cpp | ||
|---|---|---|
| 336 ↗ | (On Diff #1490) | I'd have put the whole thing into a macro and called it good enough. You can't assign all things because in some cases you'd need to do the checks before that, and using out parameters seems ugly for just a few checks. |
| source/tools/atlas/GameInterface/Handlers/CinemaHandler.cpp | ||
|---|---|---|
| 336 ↗ | (On Diff #1490) | Ah, macro, ok, I'll try. |
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!
http://jw:8080/job/phabricator/900/ for more details.
Comment Actions
Code looks, thanks for the patch.
| source/tools/atlas/GameInterface/Handlers/CinemaHandler.cpp | ||
|---|---|---|
| 413 ↗ | (On Diff #1505) | I guess the actual values here don't really matter, do they? |
| source/tools/atlas/GameInterface/Handlers/CinemaHandler.cpp | ||
|---|---|---|
| 413 ↗ | (On Diff #1505) | Yes, it's normalized, it'll matter when I'll add multi-axis moving. |