Paths

Table of Contentst

Add type conversation for CCinemaPath for TriggerScripts
ClosedPublic
Actions

Authored by vladislavbelov on Apr 11 2017, 2:49 PM.

Details

Reviewers

elexis
wraitii

Commits

rP19422: Support cinematic path creation from trigger scripts.

Summary

Allows to pass data from Js to Cpp, it'd useful for trigger's script.

Example:

	let segment = Vector3D.sub(outposts[1], outposts[0]);
	let normal = Vector3D.clone(segment).normalize();
	let distance = 80;
	[normal.x, normal.z] = [-normal.z, normal.x];
	let path = {
		"name": "test" + Math.random(),
		"orientation": "target",
		"positionNodes": [
			{"deltaTime": 0, "position": Vector3D.add(Vector3D.add(outposts[0], Vector3D.mult(normal, distance)), new Vector3D(0, 40, 0))},
			{"deltaTime": 2 + segment.length() / 10, "position": Vector3D.add(Vector3D.add(outposts[1], Vector3D.mult(normal, distance)), new Vector3D(0, 40, 0))}
		],
		"targetNodes": [
			{"deltaTime": 0, "position": Vector3D.add(outposts[0], new Vector3D(0, 3, 0))},
			{"deltaTime": 1, "position": Vector3D.add(outposts[0], new Vector3D(0, 9, 0))},
			{"deltaTime": segment.length() / 10, "position": Vector3D.add(outposts[1], new Vector3D(0, 9, 0))},
			{"deltaTime": 1, "position": Vector3D.add(outposts[1], new Vector3D(0, 3, 0))}
		]
	};

	cmpCinemaManager.AddPath(path);
	cmpCinemaManager.AddCinemaPathToQueue(path.name);
	cmpCinemaManager.Play();

Test Plan

Open game
Load the "Cinema Demo 2" map
Test that all work correctly

Diff Detail

Repository

rP 0 A.D. Public Repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

Cinema_Demo_2.zip3 KBDownload

elexis awarded a token.Apr 11 2017, 3:20 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/745/ for more details.

Harbormaster completed remote builds in B1093: Diff 1211.Apr 11 2017, 3:58 PM

leper added a subscriber: leper.Apr 11 2017, 9:19 PM

leper added inline comments.

source/simulation2/scripting/EngineScriptConversions.cpp
22 ↗	(On Diff #1211)	I'd prefer if the simulation related things in there are moved to the simulation first, then add the conversion. Else someone in the future might see this as an example.

vladislavbelov added inline comments.Apr 11 2017, 9:30 PM

source/simulation2/scripting/EngineScriptConversions.cpp
22 ↗	(On Diff #1211)	Hmm, if you care about that then it needs to remove some data from conversations, because there are already `graphics/Color.h, graphics/Shapes.h`, which aren't simulation data only. So, I don't think, that it's reason to not add the conversation.

elexis added inline comments.Apr 12 2017, 12:36 PM

source/simulation2/scripting/EngineScriptConversions.cpp
22 ↗	(On Diff #1211)	Including classes from ps/ and maths/ in the simulation is one thing, including stuff from graphics/ another. Either if this diff is orthogonal to the CinemaPath moving, then it doesn't cost anything to do the moving beforehand. If this diff needs to be significantly rewritten if moving afterwards, then it's bad to merge this before.

elexis requested changes to this revision.Apr 12 2017, 12:36 PM

This revision now requires changes to proceed.Apr 12 2017, 12:36 PM

vladislavbelov requested review of this revision.Apr 12 2017, 1:09 PM

vladislavbelov edited edge metadata.

vladislavbelov added inline comments.

source/simulation2/scripting/EngineScriptConversions.cpp
22 ↗	(On Diff #1211)	I think you're misunderstanding graphics dir, there shouldn't be data from renderer, which is realtime. But graphic is simulation part too: colors, shapes, etc. CinemaPath should be in graphics, because it's about graphic, not clear math, not common think like ps. Also CinemaPath is used only for simulation and not anywhere else. CinemaManager only calls a component. So the diff should be changed in this way.

This revision now requires changes to proceed.Apr 12 2017, 1:09 PM

vladislavbelov added inline comments.Apr 12 2017, 1:26 PM

source/simulation2/scripting/EngineScriptConversions.cpp
22 ↗	(On Diff #1211)	*shouldn't be changed

vladislavbelov added a child revision: D324: Move CCinemaPath to simulation2/helpers.Apr 13 2017, 1:55 AM

elexis edited the summary of this revision. (Show Details)Apr 15 2017, 1:59 AM

Promising demo map, I hope we can use it for Danubius D204 or similar :-)

Was wondering whether we would group the checks by the option, i.e. like

	bool hasName;
	JS::RootedValue name(cx);
	if (!JS_HasProperty(cx, obj, "name", &hasName))
		FAIL("Failed to get CCinemaPath.name property");
	if (!hasName || !JS_GetProperty(cx, obj, "name", &name) || !FromJSVal(cx, name, pathData.m_Name))
		FAIL("Failed to get CCinemaPath.name property");

	bool hasOrientation;
	JS::RootedValue orientation(cx);
	if (!JS_HasProperty(cx, obj, "orientation", &hasOrientation))
		FAIL("Failed to get CCinemaPath.orientation property");
	if (!hasOrientation || !JS_GetProperty(cx, obj, "orientation", &orientation) || !FromJSVal(cx, orientation, pathData.m_Orientation))
		FAIL("Failed to get CCinemaPath.orientation property");

	bool hasPosition;
	JS::RootedValue position(cx);
	if (!JS_HasProperty(cx, obj, "positionNodes", &hasPosition))
		FAIL("Failed to get CCinemaPath.positionNodes property");
	if (!hasPosition || !JS_GetProperty(cx, obj, "positionNodes", &position))
		FAIL("Failed to get CCinemaPath.positionNodes property");

But you have already proposed an even cleaner solution, so we'll see.

source/simulation2/scripting/EngineScriptConversions.cpp
22 ↗	(On Diff #1211)	Thanks for addressing this particular concern in D324. (This include path has to be updated when committing)
302 ↗	(On Diff #1211)	Both comment lines seem unneeded
354 ↗	(On Diff #1211)	trailing whitespace
360 ↗	(On Diff #1211)	that line is a bit long (175 chars, coding convention recommends 80, 120 could be considered extended tolerance) and having those properties on a new line each gives a nice symmetry.

vladislavbelov added a child revision: D338: Add helper for the getting of an object's property.Apr 15 2017, 7:12 PM

elexis mentioned this in D338: Add helper for the getting of an object's property.Apr 17 2017, 1:46 AM

Used new helper FromJsProperty

Build has FAILED

Updating workspaces.
Build (release)...
../../../source/simulation2/scripting/EngineScriptConversions.cpp: In static member function ‘static bool ScriptInterface::FromJSVal(JSContext*, JS::HandleValue, T&) [with T = TNSpline; JS::HandleValue = JS::Handle<JS::Value>]’:
../../../source/simulation2/scripting/EngineScriptConversions.cpp:325:55: error: ‘FromJsProperty’ was not declared in this scope
   if (!FromJsProperty(cx, node, "deltaTime", deltaTime))
                                                       ^
../../../source/simulation2/scripting/EngineScriptConversions.cpp:329:53: error: ‘FromJsProperty’ was not declared in this scope
   if (!FromJsProperty(cx, node, "position", position))
                                                     ^
../../../source/simulation2/scripting/EngineScriptConversions.cpp: In static member function ‘static bool ScriptInterface::FromJSVal(JSContext*, JS::HandleValue, T&) [with T = CCinemaPath; JS::HandleValue = JS::Handle<JS::Value>]’:
../../../source/simulation2/scripting/EngineScriptConversions.cpp:350:52: error: ‘FromJsProperty’ was not declared in this scope
  if (!FromJsProperty(cx, v, "name", pathData.m_Name))
                                                    ^
../../../source/simulation2/scripting/EngineScriptConversions.cpp:353:66: error: ‘FromJsProperty’ was not declared in this scope
  if (!FromJsProperty(cx, v, "orientation", pathData.m_Orientation))
                                                                  ^
../../../source/simulation2/scripting/EngineScriptConversions.cpp:356:60: error: ‘FromJsProperty’ was not declared in this scope
  if (!FromJsProperty(cx, v, "positionNodes", positionSpline))
                                                            ^
../../../source/simulation2/scripting/EngineScriptConversions.cpp:359:95: error: ‘FromJsProperty’ was not declared in this scope
  if (pathData.m_Orientation == L"target" && !FromJsProperty(cx, v, "targetNodes", targetSpline))
                                                                                               ^
../../../source/simulation2/scripting/EngineScriptConversions.cpp:363:62: error: ‘FromJsProperty’ was not declared in this scope
  if (!FromJsProperty(cx, v, "timescale", pathData.m_Timescale))
                                                              ^
../../../source/simulation2/scripting/EngineScriptConversions.cpp:365:52: error: ‘FromJsProperty’ was not declared in this scope
  if (!FromJsProperty(cx, v, "mode", pathData.m_Mode))
                                                    ^
../../../source/simulation2/scripting/EngineScriptConversions.cpp:367:54: error: ‘FromJsProperty’ was not declared in this scope
  if (!FromJsProperty(cx, v, "style", pathData.m_Style))
                                                      ^
make[1]: *** [obj/simulation2_Release/EngineScriptConversions.o] Error 1
make: *** [simulation2] Error 2
make: *** Waiting for unfinished jobs....

Link to build: http://jw:8080/job/phabricator/792/
See console output for more information: http://jw:8080/job/phabricator/792/console

Harbormaster failed remote builds in B1150: Diff 1294!Apr 17 2017, 2:47 AM

Thanks for the update, with the D338 helper the code becomes much shorter and readable!

There is no warning if properties are passed that are not being used. It could give some user feedback but is not mandatory (and Vladislav has stated one or two days ago in IRC that he like it this way).

Spline conversion completeness:

Rotation nodes will be implemented as a custom spline, so at that time we shouldn't forget to implement them in this place too, if the support for them isn't completely dropped (hence the CFixedVector3D() currently).
The spline velocity is computed in BuildSpline and there is no setter, so it's correct to not add support in this conversion.

CinemaPath conversion completeness:

We receive 2 splines and CCinemaData. See above for the former. For the latter all members can be set from JS, besides m_LookAtTarget which is set automatically in the constructor.

The distortion variables m_GrowthCount, m_Growth, m_Switch can't be set from JS yet, but Vladislav "plans to add some refractoring, so didn't add it as params", which is fine with me.

Also did the OOS litmus test and the prior serialization work still holds (even though the graphical aspect is not in sync yet).
As mentioned above, nice feature and I hope we can use it for D204 or some other map in this release!

source/simulation2/scripting/EngineScriptConversions.cpp
307 ↗	(On Diff #1294)	can be moved below the early fail
323 ↗	(On Diff #1294)	\n
330 ↗	(On Diff #1294)	\n
364 ↗	(On Diff #1294)	\n and following lines too
353 ↗	(On Diff #1211)	Could be moved just before its usage

This revision is now accepted and ready to land.Apr 17 2017, 3:38 AM

Closed by commit rP19422: Support cinematic path creation from trigger scripts. (authored by elexis). · Explain WhyApr 17 2017, 3:42 AM

This revision was automatically updated to reflect the committed changes.

elexis retitled this revision from Add type conversation for CCinemaPath to Add type conversation for CCinemaPath for TriggerScripts.Apr 17 2017, 3:43 AM

Revision Contents
Changeset List

Path

Size

ps/

trunk/

source/

simulation2/

components/

ICmpCinemaManager.cpp

1 line

scripting/

EngineScriptConversions.cpp

76 lines

Diff 1295

View Options

ps/trunk/source/simulation2/components/ICmpCinemaManager.cpp