#3301 closed task (fixed)
[PATCH] Cinematic camera
Reported by: | Vladislav Belov | Owned by: | Vladislav Belov |
---|---|---|---|
Priority: | Should Have | Milestone: | Alpha 20 |
Component: | Core engine | Keywords: | patch |
Cc: | Patch: |
Description (last modified by )
Cinematic camera needs for cutscenes, game videos, etc. There is some cinematic camera realization, but it's uncompleted and can't be used in map.
After modification camera paths have next structure (in map.xml):
<Paths> <Path name="test" timescale="1" orientation="target" mode="ease_inout" style="default"> <Node time="0"> <Position x="485.029" y="25.8692" z="866.186"/> <Target x="0" y="0" z="0"/> </Node> <!-- other nodes --> </Path> </Paths>
Mode & style are params for the camera moving near nodes. And from the map trigger we can add a path to the playlist by name.
The draft of patch I will attach later. And Atlas Paths editing after trompetin17 AtlasUI merging.
Example of cutscene with cinematic camera: http://www.youtube.com/watch?v=xkLrpkow7XU.
Attachments (35)
Change History (73)
by , 9 years ago
Attachment: | camera_path_visual.jpg added |
---|
by , 9 years ago
Attachment: | Cinema_Demo_map.zip added |
---|
by , 9 years ago
Attachment: | cinematic_camera_core_v0.5.patch added |
---|
by , 9 years ago
Attachment: | cinematic_camera_core_v0.5a.patch added |
---|
comment:1 by , 9 years ago
Can't compile it, since the class CCinemaManager
is defined in CinemaManager.h
and CinemaTrack.h
. Perhaps the latter is supposed to be removed?
by , 9 years ago
Attachment: | cinematic_camera_core_v0.5b.patch added |
---|
comment:2 by , 9 years ago
Keywords: | patch added |
---|---|
Summary: | [WIP] Cinematic camera → [PATCH] Cinematic camera |
Thanks, that compiles.
Actually I'm surprised of how good that looks, especially with that 16:9 format.
Should we have some skyline (new ticket), as the black background looks a bit unnatural? Also we should make sure that the low-res textures aren't in view for too long (in that case the gate looks a bit ugly).
Linking a video of that cutscene here would also help promoting the patch I guess.
With regards to the code:
--i;
instead ofi--;
(NUSpline.cpp
)--m_GrowthCount
(CinemaPath.cpp
)- trailing whitespace in an empty line in
CXMLReader::ReadPaths
- some unused variables (don't know if you will use them later)
- The patch could be split into smaller parts (maybe not needed if leper reviews it ;) ). For example the cleanup of existing code could be done in an individual commit. Also I saw that you moved some code. Might be easier to review if the code is not be moved but edited in place. Make sure to ask leper before you follow these proposals.
Good job!
comment:3 by , 9 years ago
To undo the patch, make sure to remove the newly added files:
rm source/graphics/CinemaManager.cpp rm source/graphics/CinemaManager.h rm source/graphics/CinemaPath.cpp rm source/graphics/CinemaPath.h rm source/simulation2/components/CCmpCinemaManager.cpp rm source/simulation2/components/ICmpCinemaManager.cpp rm source/simulation2/components/ICmpCinemaManager.h
follow-up: 11 comment:4 by , 9 years ago
I had a look at this patch today because I'm working on a kind of a benchmark mode. It's quite cool and works well, good work!
Some feedback:
- I get some OpenAL errors when I try to set more than two nodes in a path with the same position but different rotation.
- It would be nice to use this for random maps too (define camera paths from code).
- Adding OnCinemaPathEnded as a Trigger event would be useful (I think that was the idea, but I'll mention it anyway).
- There's some deprecated SpiderMonkey code. Use JS::ObjectValue(*obj) instead of OBJECT_TO_JSVAL(obj) and JS::Value instead of jsval for new code. In some version they will drop support for the old API.
comment:5 by , 9 years ago
Milestone: | Backlog → Alpha 20 |
---|
As discussed on IRC Vlad is still working on it, so I'm pushing this to A20
by , 9 years ago
Attachment: | cinematic_camera_core_v0.6.patch added |
---|
comment:6 by , 9 years ago
Description: | modified (diff) |
---|
comment:7 by , 9 years ago
Description: | modified (diff) |
---|
by , 8 years ago
Attachment: | cinematic_camera_core_v0.7.patch added |
---|
Small fixes, draw target spline, few renamings
by , 8 years ago
Attachment: | cinematic_camera_core_v0.8.patch added |
---|
OpenAL error fix, debug_printf fix, target without position fix
by , 8 years ago
Attachment: | Cinema_Demo_2.zip added |
---|
Test map for target path and event CinemaPathEnded
by , 8 years ago
Attachment: | cinematic_camera_core_v0.9.patch added |
---|
Small fixes, trigger event for CinemaPathEnded
by , 8 years ago
Attachment: | cinematic_camera_core_v0.9b.patch added |
---|
Indetation fix, deprecated js fix
by , 8 years ago
Attachment: | cinematic_camera_core_v1.0.patch added |
---|
Disable mouse input, small fixes
comment:8 by , 8 years ago
Keywords: | review added |
---|
comment:9 by , 8 years ago
We have already discussed this a bit on IRC today. There's a fundamental problem about the question what is network synchronized and what not. The message CMessageCinemaPathEnded is triggered based on the real time passed on clients or if the user presses the escape button. This means that message can't be used for anything useful in the simulation or it will break multiplayer (Out of sync errors) or things like replays or saved games in single player mode.
Either all the simulation messages are triggered only based on network synchronized simulation time or you remove that message. However, such messages could be quite useful for campaign designers. Otherwise they have to work with timers.
It's good to do the camera movement based on realtime because it allows smooth camera movement. That should not be changed!
We also discussed the question if players should be allowed to skip the video sequence in multiplayer games. In my opinion this should be allowed. However, it must not trigger a simulation message early when one player skips the video because this could cause out of sync errors.
In my opinion, the best solution is to calculate the state of the cinematic playback twice. First, in the simulation using network synchronized simulation time and second, outside of the simulation using real time. This allows smooth camera movement the way it works now and provides more or less accurate network synchronized events to the simulation and especially triggers scripts. If the user skips playback, it would only stop the calculations outside of the simulation and would not change anything about the CinemaPathEnded message. This message could still be used to trigger actions that come after the playback has ended.
comment:10 by , 8 years ago
Keywords: | review removed |
---|
comment:11 by , 8 years ago
by , 8 years ago
Attachment: | cinematic_camera_core_v1.1.patch added |
---|
Control for the cinematic from the GUI (skip, pause)
by , 8 years ago
Attachment: | cinematic_camera_core_v1.2.patch added |
---|
Fixed data types for a simdata, serializing the CinemaManager component
by , 8 years ago
Attachment: | cinematic_camera_core_v1.4.patch added |
---|
Serialize & deserialize implemented
comment:12 by , 8 years ago
Keywords: | review added |
---|
by , 8 years ago
Attachment: | cinematic_camera_core_v1.4b.patch added |
---|
Disable events while for the normal sync
follow-up: 14 comment:13 by , 8 years ago
I've started with the review. There are still a few issues with simulation synchronization.
You serialize the following data: p_CinematicSimulationData->m_Paused: This means that cinematic has to be paused for all players simultaneously or it will go out of sync. In my opinion it's not necessary to store the paused state of a cinematic.
p_CinematicSimulationData->m_MapRevealed: What if one players skips the cinematic? Will his map be revealed while the cinematic is till running for other players? This should not be stored in the simulation state.
You can also reproduce an out of sync error if you start your Cinema Demo 2 map in multiplayer and then pause one of the games. It doesn't happen instantly, probably because we don't check the full hash in each turn. You can start two instances of 0 A.D. in windowed mode on a single computer and test multiplayer this way.
There are also some compiler errors and warnings. Compiler errors: Use u32 instead of size_t. Compiler warnings about unused variables: We want 0 A.D. to compile without warnings, so these should be fixed or avoided before committing. Some are just because of functions that aren't implemented yet, but others are real.
0ad/source/simulation2/components/CCmpCinemaManager.cpp||In member function ‘virtual void CCmpCinemaManager::Deserialize(const CParamNode&, IDeserializer&)’:| 0ad/source/simulation2/components/CCmpCinemaManager.cpp|129|error: no matching function for call to ‘IDeserializer::NumberU32_Unbounded(const char [14], size_t&)’| 0ad/source/simulation2/components/CCmpCinemaManager.cpp|129|note: candidate is:| ../../../source/simulation2/serialization/IDeserializer.h|49|note: virtual void IDeserializer::NumberU32_Unbounded(const char*, uint32_t&)| ../../../source/simulation2/serialization/IDeserializer.h|49|note: no known conversion for argument 2 from ‘size_t {aka long unsigned int}’ to ‘uint32_t& {aka unsigned int&}’| 0ad/source/simulation2/components/CCmpCinemaManager.cpp|143|error: no matching function for call to ‘IDeserializer::NumberU32(const char [14], size_t&, int, int)’| 0ad/source/simulation2/components/CCmpCinemaManager.cpp|143|note: candidate is:| ../../../source/simulation2/serialization/IDeserializer.h|43|note: virtual void IDeserializer::NumberU32(const char*, uint32_t&, uint32_t, uint32_t)| ../../../source/simulation2/serialization/IDeserializer.h|43|note: no known conversion for argument 2 from ‘size_t {aka long unsigned int}’ to ‘uint32_t& {aka unsigned int&}’| 0ad/source/simulation2/components/CCmpCinemaManager.cpp|163|error: no matching function for call to ‘IDeserializer::NumberU32(const char [20], size_t&, int, int)’| 0ad/source/simulation2/components/CCmpCinemaManager.cpp|163|note: candidate is:| ../../../source/simulation2/serialization/IDeserializer.h|43|note: virtual void IDeserializer::NumberU32(const char*, uint32_t&, uint32_t, uint32_t)| ../../../source/simulation2/serialization/IDeserializer.h|43|note: no known conversion for argument 2 from ‘size_t {aka long unsigned int}’ to ‘uint32_t& {aka unsigned int&}’| 0ad/source/simulation2/components/CCmpCinemaManager.cpp||In member function ‘virtual void CCmpCinemaManager::HandleMessage(const CMessage&, bool)’:| 0ad/source/simulation2/components/CCmpCinemaManager.cpp|190|warning: unused variable ‘msgData’ [-Wunused-variable]| ||=== Build finished: 3 error(s), 1 warning(s) (0 minute(s), 46 second(s)) ===|
0ad/source/simulation2/components/CCmpCinemaManager.cpp||In member function ‘virtual void CCmpCinemaManager::HandleMessage(const CMessage&, bool)’:| 0ad/source/simulation2/components/CCmpCinemaManager.cpp|190|warning: unused variable ‘msgData’ [-Wunused-variable]| 0ad/source/graphics/CinemaManager.cpp|83|warning: unused parameter ‘name’ [-Wunused-parameter]| 0ad/source/graphics/CinemaManager.cpp|83|warning: unused parameter ‘current’ [-Wunused-parameter]| 0ad/source/graphics/CinemaManager.cpp|136|warning: unused parameter ‘time’ [-Wunused-parameter]| 0ad/source/graphics/CinemaManager.cpp|174|warning: unused parameter ‘skipped’ [-Wunused-parameter]| 0ad/source/graphics/MapReader.cpp||In member function ‘void CXMLReader::ReadPaths(XMBElement)’:| 0ad/source/graphics/MapReader.cpp|844|warning: unused variable ‘el_distortion’ [-Wunused-variable]| 0ad/source/graphics/MapReader.cpp|849|note: in expansion of macro ‘EL’| 0ad/source/graphics/MapReader.cpp|845|warning: unused variable ‘at_growth’ [-Wunused-variable]| 0ad/source/graphics/MapReader.cpp|858|note: in expansion of macro ‘AT’| 0ad/source/graphics/MapReader.cpp|845|warning: unused variable ‘at_switch’ [-Wunused-variable]| 0ad/source/graphics/MapReader.cpp|859|note: in expansion of macro ‘AT’|
comment:14 by , 8 years ago
Replying to Yves:
I've started with the review. There are still a few issues with simulation synchronization.
m_Paused, because if one pause then all pause, else sim will stopped, but camera will be still moving. How I s better to implement that? m_RevealedMap, not changing after end, it's state, map is revealed or not before start cinematic.
Ok, I will fixed this warnings (btw I don't have warnings & errors, at least the last patch, last patch was compiled on the visual studio)
by , 8 years ago
Attachment: | cinematic_camera_core_v1.5b.patch added |
---|
Init value of m_MapRevealed fix
by , 8 years ago
Attachment: | cinematic_camera_core_v1.5c.patch added |
---|
Remove m_Pause from serializing
follow-up: 16 comment:15 by , 8 years ago
As discussed on IRC, the main issue is that data in the CinematicSimulationData struct is modified by user input directly, which causes Out of Sync (OOS) problems.
The suggested solution is to trigger events such as "NodeReached" or "PathEnded" based on timers in the simulation. These events can then be used by triggers safely. All serialized data should be modified only from the simulation. The rendering part calculates the camera movement precisely and fluently and may use non-deterministic data such as floats or user input. It does not modify any data that gets serialized and is only responsible for the visualization.
Skipping paths gets implemented on the visualization part and does not influence the simulation. The timers keep running in the background, but the visualization of the cinematic is skipped. We can still implement a simulation flag like "PreventCinematicSkipping" in the future if we don't want to allow skipping of cinematics in multiplayer games.
Pausing has to work different. Because pausing also pauses the simulation, it has to be network synchronized. However, we currently don't have pausing implemented properly in the game for multiplayer games (it just works more or less because the game does not continue if one player stops responding because his client is paused). For this reason, pausing can either not be implemented yet in this patch or it can be purely visual too (it's a bit weird in the multiplayer case but doesn't break it and should work fine for singleplayer mode)
comment:16 by , 8 years ago
Replying to Yves:
As discussed on IRC, the main issue is that data in the CinematicSimulationData struct is modified by user input directly, which causes Out of Sync (OOS) problems.
Patch has the CinemaPathEnded event already, so it needs only the CinemaNodeReached event.
First we need to improve the pause/enable, and in the next patches skipping ability (so skipping can be disable at current moment).
by , 8 years ago
Attachment: | cinematic_camera_core_v1.6.patch added |
---|
Simulation fix, m_Enabled checking in the MT_Update
follow-up: 18 comment:17 by , 8 years ago
I still see that Skip() can be triggered by a hotkey and then it will call Next() and this will do m_CinematicSimulationData.m_PathQueue.pop_front(); So actions from the GUI (or hotkeys) influence the simulation state directly which can cause OOS issues.
Some additional (minor) things I've found:
- Why are there two different functions for Next() and Skip()?
- NumberFloat_Unbounded used in CCmpCinemaManager.cpp line 108? Should that be changed to NumberFixed_Unbounded and fixed::Zero()?
- Why are lines 89-91 of CCmpCinemaManager.cpp duplicated on lines 93-95?
- I'll fix the comment in Trigger.js in a separate commit (the Training/Research thing)
- Compiler warning CCmpCinemaManager.cpp|187|warning: ‘msg’ may be used uninitialized in this function [-Wmaybe-uninitialized]|
- the local variable msg should be renambed to msgData. This avoids the warning and also confusion becasue the parameter has the same name.
comment:18 by , 8 years ago
Replying to Yves:
I still see that Skip() can be triggered by a hotkey and then it will call Next() and this will do m_CinematicSimulationData.m_PathQueue.pop_front();
As I replied in the irc, queue doesn't sync until, so it's ok.
- Skip just internal function, probably it will be extended or removed.
- Fixed (probably, it was just for speed, I think).
- It's different, in the future Rotate will be other spline.
- Good.
- Fixed.
by , 8 years ago
Attachment: | cinematic_camera_core_v1.7.patch added |
---|
Patch rebased on the current SVN
comment:21 by , 8 years ago
Please don't add additional styles fixed (whitespaces and similar) now. It's important for the commit that real changes in functionality are separated from style fixes.
Some additional things I've noticed during the review:
- AddPath: Use "const CCinemaPath&" for CCinemaPath parameter
- void CCinemaManager::Next(bool UNUSED(skipped)): The current implementation there is wrong, so it should be removed (replace with a "TODO: implement" comment or similar).
- Adjust include order (Foo.h should come first in Foo.cpp as defined in the wiki Coding_Conventions)
- fixed::FromFloat(0.0f) in some places ... better fixed::Zero()
- CMessageCinemaPathEnded: remove skipped property because it's not used yet and it's unclear if it will ever be used there(also in MessageTypeConversions.cpp)
- 2 comments in MessageTypes.h: These are superfluous. For a message called CMessageCinemaPathEnded it's obvious that it is sent when the path has ended.
- It appears to me that the Pause function of the CinemaManager doesn't work. Pause sets CinemaManger::m_Paused to true, but CCinemaManager::Update sets it to false again if g_Game->m_Paused is different.
- There's a similar problem with play. It works because "kind of" because it also sets m_Enabled and causes the cinematic to start this way, but it also sets m_Paused to false, which is currently useless.
- What's the purpose of the Pause function anyway? Why do other components (or trigger scripts) have to pause the cinematic? The same question applies to the Stop function. Maybe I'm missing something.
- I've found the following comment. We have too many TODO comments anyway, so either rename that or don't rename it, but don't add a todo ;). TODO: rename to TotalDistance
Open issues/todos for the future:
- Playback issue: waiting at the end and before closing cinematic mode?
- 20 sec path.
- expected update: each 200ms
- real update: 210 ms average (e.g. because of lag in multiplayer)
- 100 turns
- Path will be removed after 21 seconds
- Screen will stay frozen for 1 sec at end of path
- Playback issue: stopping before the end of the path
- Similar as the example above, just with the real update time earlier than the expected 200 ms.
- Proper handling of accelerated simulation (game speed option)
- If the game ends before a cutscene ends, the dialog shows but is not clickable
- Also adjust the Atlas related code that is still there (I think you are working on that already).
- Add a function to add new paths from trigger scripts (or other sim components).
My TODOS:
- Another style-only commit for CinemaPath.cpp (there are still some cleanup-only changes in the diff).
- I'm through with the review of most files, but not with all yet
by , 8 years ago
Attachment: | cinematic_camera_core_v1.8c.patch added |
---|
Clean-up, path drawing control fix
comment:23 by , 8 years ago
It's committed in r17594, congratulations! I forgot to reference this ticket in the commit message.
comment:24 by , 8 years ago
Hi, I tried the cinematics with the 2 demos here, it looks really nice. Here are some findings:
- demo1: when the cutscene finishes I still see the starting cutscene position for a fraction time before returning to game mode; it doesn't always happen;
- DONE in r17607:
the sky should probably be forced on during cutscenes; - demo2 gives this warning when finished and if I press no when asked to quit the menu is not available (have to exit with ALT+F4):
WARNING: Cinema path has ended. WARNING: JavaScript warning: maps/scenarios/Cinema_Demo_2.js line 13 reference to undefined property data.skipped WARNING: Path name: test skipped: undefined
- DONE:
you may want to update Triggers wiki page with the new triggers; - mouse cursor should be disabled during cutscenes; also if you are selecting units when cutscene start you see the selection rectangle during all the cutscene;
- if I run at double speed (more options setting) the cutscene play at 1x speed but terminates before (probably at half time); possible solution is to reset speed to 1x when on cutscene mode;
Possible future ideas probably worth a new ticket:
- add cutscene on every game ending (either win or loose):
- 1) simulation stops
- 2) human units change to cheering animation
- 3) camera moves over winning soldiers or other interesting hotspots until
- 4) player press the button to continue or quit
- add cutscene on game start
- 1) simulation stops
- 2) camera moves from the CC up to the traditional camera starting position using some path (a spiral around the CC)
- 3) simulation start again
- show a cutscene the first time the game is run (or when pressing some menu button)
comment:25 by , 8 years ago
About the sky, a possible option is to change default.cfg and enable it by default. Users can still disable it in option menu. This is probably better rather than forcing it always enabled during cutscene, so that users can still disable it if misbehaves. DONE in r17607.
comment:28 by , 8 years ago
Keywords: | review removed |
---|
follow-up: 30 comment:29 by , 8 years ago
At least a demo map should be committed before a20 to showcase this.
- demo1 is nice, a minor issue is that walls and other buildings in the player base belong to gaia rather than the player;
- demo2 is broken.
comment:30 by , 8 years ago
Replying to fabio:
At least a demo map should be committed before a20 to showcase this.
Demo1 isn't really playable and it doesn't use target nodes. So I think it needs to create the smaller map, with cinema on trigger (i.e. the hero come to a tower).
by , 8 years ago
Attachment: | cinematic_camera_fix.patch added |
---|
Hide cursor on the cinema playing, target save fix
comment:32 by , 8 years ago
Keywords: | review added |
---|
comment:33 by , 8 years ago
Status: | new → assigned |
---|
comment:34 by , 8 years ago
Keywords: | review removed |
---|
The patch can be split into two commits (the files changing the cursor and then the implementation of saving camera-paths to the maps in MapWriter.cpp
).
The MapWriter.cpp
part is outdated (already?) and needs to be rebased.
The cursor part is not working correctly for my system (ubuntu 15). It is hidden initially, but when I move the mouse, the cursor becomes visible again.
comment:35 by , 8 years ago
From today's IRC logs:
13:00 < Yves`> elexis, Vladislav: the way the cursor is disabled in this patch seems wrong. We already have some handling for enabling and disabling the cursor rendering. For example, the cursor is disabled when a SDL_WINDOWEVENT_LEAVE event is received. So if the CinemaManager disables the cursor rendering, it could be enabled again by a similar event. There's probably an additional variable needed that stores if the game wants to hide the cursor. The current variable (g_DoRenderCursor) is for handling if the rendering should happen based the current window state and similar (mouse inside window etc.), which is something different.
by , 8 years ago
Attachment: | cinematic_camera_map_saving_fix.2.patch added |
---|
comment:37 by , 8 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Closing as fixed since the release is coming soon and since the functionality has been implemented in the commits above.
- The demo map must be committed for a20 IMO, so that the users can actually see the implementation! You provided a demo maps before, so maybe we can just commit one of them before the relase, in case you don't have the time to provide a completed one.
To be done in other tickets:
- Hide the cursor while the camera path is animated
- Atlas GUI
Test version (0.5) of cinematic camera