Page MenuHomeWildfire Games

Add type conversation for CCinemaPath for TriggerScripts
ClosedPublic

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

Details

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
  1. Open game
  2. Load the "Cinema Demo 2" map
  3. Test that all work correctly

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 1093
Build 1722: Vulcan BuildJenkins

Event Timeline

vladislavbelov created this revision.Apr 11 2017, 2:49 PM

Vulcan added a subscriber: Vulcan.Apr 11 2017, 3:58 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.

leper added inline comments.
source/simulation2/scripting/EngineScriptConversions.cpp
22

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

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

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

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

*shouldn't be changed

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

Thanks for addressing this particular concern in D324. (This include path has to be updated when committing)

302

Both comment lines seem unneeded

354

trailing whitespace

360

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.

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

elexis accepted this revision.Apr 17 2017, 3:38 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

can be moved below the early fail

323

\n

330

\n

353

Could be moved just before its usage

364

\n and following lines too

This revision is now accepted and ready to land.Apr 17 2017, 3:38 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