Page MenuHomeWildfire Games

Allow accessing prop points in the simulation.
AbandonedPublic

Authored by Stan on Jun 16 2019, 12:58 PM.

Details

Reviewers
wraitii
Summary

Currently it is not possible to access the model's prop point in the JS code which for instance makes it that you have to manually specify visible garrison prop points, while you could use the ones defined by artists in the mesh.

Test Plan

Add this block of code somewhere (ex in GarrisonHolder, or in UnitAI) to test if one can get the prop points in the simulation

	let cmpVisual = Engine.QueryInterface(this.entity, IID_Visual);
	if (cmpVisual)
		for (let prop of cmpVisual.GetPropPoints())
			warn(uneval(prop));

You can then test that for instance the rider prop point of D1958 matches roughly the one defined in the template there. Verify that values are correct.

Test that nothing broke.

Diff Detail

Event Timeline

Stan created this revision.Jun 16 2019, 12:58 PM
Stan added inline comments.
source/simulation2/scripting/EngineScriptConversions.cpp
360

There is probably a way to make it generic for every type of std::vector but I haven't found it.

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/simulation2/components/ICmpVisual.h
|   1| /*·Copyright·(C)·2018·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2018"

source/simulation2/components/ICmpVisual.cpp
|   1| /*·Copyright·(C)·2018·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2018"

source/simulation2/components/CCmpVisualActor.cpp
|   1| /*·Copyright·(C)·2018·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2018"
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1746/display/redirect

Hi! I got a segfault when testing this:

TIMER| ParseTerrain: 9.13745 ms

Thread 1 "pyrogenesis" received signal SIGSEGV, Segmentation fault.
CCmpVisualActor::Init (this=0x55555a6fd230, paramNode=...)
    at ../../../source/simulation2/components/CCmpVisualActor.cpp:212
212			std::vector<SPropPoint>& props = m_Unit->GetModel().ToCModel()->GetModelDef()->m_PropPoints;

Reproducible with attached replays.

Stan added a comment.Aug 21 2019, 11:13 AM

@Freagarach that's strange, can you try to figure out what was null and shouldn't have been ?

elexis added a subscriber: elexis.Aug 21 2019, 11:39 AM

you have to manually specify visible garrison prop points, while you could use the ones defined by artists in the mesh.

Oh wait, the use case is for the simulation?
That sounds like it might be problematic, because the simulation is supposed to be independent of the graphics data.
So not sure if this is right then.
Are there other precedences where the simulation state is dependent on actors?
From a quick look it seems that the VisualActor component determines alone what it wants to do with the actor?

In D1989#91735, @Stan wrote:

can you try to figure out what was null and shouldn't have been ?

The line number gives an indication already, there aren't so many possible null dereferences, you can print the entityID if you want to know which entity exactly, and can do g_Selection.AddList([ent]) in F9 / JS console in the GUI, or print the template name in C++ too.
Usually it's not unhealthy to check whether a pointer is null before dereferencing it. But some pointers are also null at first and initialized some time later.

source/simulation2/components/CCmpVisualActor.cpp
91
/**
   * Can be used for ...
   */
225

Why does the simulation store printable names intead of the names?

233

If you know the size of m_PropPointsPositions you can reserve the space in advance.

Also you can avoid the copy by either using emplace_back and passing the three arguments to the constructor, or default constructing with emplace_back() and then operating on the back() reference.

823

Hope that it performs copy elision or take it into your own hands to ensure that?

source/simulation2/components/ICmpVisual.h
41

I would recommend to rename the Js name because the data is not specific to JS.

98

It's not necessary to use the Fixed type for JS.
It is necessary to use the Fixed type for deterministic simulation data.
But it doesn't matter if the GUI displays something slightly different for the different players.

source/simulation2/scripting/EngineScriptConversions.cpp
354

ScriptInterface::CreateObject

360

Take a look at the line after the code you insert here.

369

(Otherwise had said ScriptInterface::CreateArray)

Stan marked an inline comment as done.Aug 21 2019, 11:56 AM

Oh wait, the use case is for the simulation?
That sounds like it might be problematic, because the simulation is supposed to be independent of the graphics data.
So not sure if this is right then.
Are there other precedences where the simulation state is dependent on actors?
From a quick look it seems that the VisualActor component determines alone what it wants to do with the actor?

Yeah basically it handles everything, sets variants and whatnot. However I'm not sure it really affects the simulation more than having those values in templates.
It would really come in handy when/if we add turret support for units. I don't see myself having to edit all the cavalry / camelry/ elephantry template to place their riders when we add turrets.

The line number gives an indication already, there aren't so many possible null dereferences, you can print the entityID if you want to know which entity exactly, and can do g_Selection.AddList([ent]) in F9 / JS console in the GUI, or print the template name in C++ too.
Usually it's not unhealthy to check whether a pointer is null before dereferencing it. But some pointers are also null at first and initialized some time later.

Yeah I guess. I was just inquiring to maybe gain a little time.

source/simulation2/components/CCmpVisualActor.cpp
91
/**
 * Can be used to retrieve special locations given by the artists
 * to place actors with precision on this actor.
 */
225

That's the only function I found that returns a std::string.

233

So something like

m_PropPointsPositions.resize(props.size())

Also you can avoid the copy by either using emplace_back and passing the three arguments to the constructor, or default constructing with emplace_back() and then operating on the back() reference.

Do you have an example of the latter ?

823

Likely not since I don't know what https://en.cppreference.com/w/cpp/language/copy_elision is :)

source/simulation2/components/ICmpVisual.h
41

Well it will only be used there, so I thought it made sense.

98

In this case since it will affect simulation it needs to fixed.

source/simulation2/scripting/EngineScriptConversions.cpp
369

What are the advantages / disadvantages ?

In D1989#91742, @elexis wrote:

That sounds like it might be problematic, because the simulation is supposed to be independent of the graphics data.
So not sure if this is right then.
Are there other precedences where the simulation state is dependent on actors?

I think projectiles start at the first ammo point, which is defined in the actors, not in templates. That seems to make sense to me - so that's a strong precedent that prop-points are part of the simulation.

Stan updated this revision to Diff 9951.EditedSep 23 2019, 9:59 PM
Stan marked 6 inline comments as done.
  • Use vector macro
  • Use CreateObject
  • Use emplace back
  • Hopefully fix copy elision
  • Add null check

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/824/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/309/display/redirect

Stan marked 2 inline comments as done.Sep 24 2019, 8:14 AM
Stan added inline comments.
source/simulation2/scripting/EngineScriptConversions.cpp
369

Nevermind. Got it ^^'

Stan updated this revision to Diff 9952.Sep 24 2019, 12:32 PM
  • Fix build
  • Remove failed attempt at copy ellision.

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/310/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/825/display/redirect

Stan updated this revision to Diff 9953.Sep 24 2019, 2:25 PM
  • Add the other conversion required by the macro.

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/311/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/simulation2/components/ICmpVisual.h
|  39| »   std::string·m_Name;
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'std::string' is invalid C code. Use --std or --language to configure the language.

source/simulation2/scripting/EngineScriptConversions.cpp
| 174| »   »   FAIL_VOID("Failed·to·get·Vector3D·constructor");
|    | [MAJOR] CPPCheckBear (unknownMacro):
|    | There is an unknown macro here somewhere. Configuration is required. If STMT is a macro then please configure it.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/826/display/redirect

Stan updated this revision to Diff 9974.EditedSep 27 2019, 11:27 AM
  • Use bones relative position when available.

@Freagarach I can't find where the minus sign might be coming from, maybe it's computed using rotation ? I should probably pass that as well.

Current

x:0.0060272216796875, y:1.230453491210937, z:2.85987854003

Previous:

x:0.0060272216796875, y:0.075897216796875, z:1.31005859375

Objective

x:0.0000000000000000, y:1.400000000000000, z:-2.50000000000

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/324/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/simulation2/components/ICmpVisual.h
|  39| »   std::string·m_Name;
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'std::string' is invalid C code. Use --std or --language to configure the language.

source/simulation2/scripting/EngineScriptConversions.cpp
| 174| »   »   FAIL_VOID("Failed·to·get·Vector3D·constructor");
|    | [MAJOR] CPPCheckBear (unknownMacro):
|    | There is an unknown macro here somewhere. Configuration is required. If STMT is a macro then please configure it.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/839/display/redirect

Stan added a comment.Sep 29 2019, 10:47 PM

No I did not I'll see if that bring back the minus. Still lies the issue of non visual...

Test Plan: Test that nothing broke.

I assume this sentence is written because Phabricator refuses empty test fields.

See irc comments http://irclogs.wildfiregames.com/2019-09/2019-09-29-QuakeNet-%230ad-dev.log:

20:50 < elexis> FromJSVal<JsProp> wont work
20:50 < elexis> I suppose you can delete the function, I assume its not used anywhere
20:53 < elexis> if GetPropPoints will be used for data that will change the simulation state then you gotta be careful that the Fixed type will be applied before any operation, seems to be the case, if its already working for the projectile launch point
20:53 < elexis> though I didnt check the code to see if that is only graphical or sim state influencing
20:55 < elexis> if it does influence simstate, perhaps SPropPoint should be moved to sim and transformed to use Fixed vectors
20:56 < elexis> EscapeToPrintableASCII doesnt sound right, since youre not only gonna print the name
20:56 < elexis> if its only graphical, then SPropPoint can be used without modification
In D1989#91742, @elexis wrote:

That sounds like it might be problematic, because the simulation is supposed to be independent of the graphics data.
So not sure if this is right then.
Are there other precedences where the simulation state is dependent on actors?

I think projectiles start at the first ammo point, which is defined in the actors, not in templates. That seems to make sense to me - so that's a strong precedent that prop-points are part of the simulation.

Is it true? Where's the code for that? If it is true, then I suppose one can check how that code does it and evaluate whether that was more lucky that it worked. Its not the first time we have simulation data specified in graphics/ (terrain data too for instance).
Well looking at the code, I find that it wasnt introduced until rP20676, with

Attack.prototype.PerformAttack = function(type, target)
{
....

		// TODO: Use unit rotation to implement x/z offsets.
		let deltaLaunchPoint = new Vector3D(0, +this.template[type].Projectile.LaunchPoint["@y"], 0);
		let launchPoint = Vector3D.add(selfPosition, deltaLaunchPoint);

		let cmpVisual = Engine.QueryInterface(this.entity, IID_Visual);
		if (cmpVisual)
		{
			// if the projectile definition is missing from the template
			// then fallback to the projectile name and launchpoint in the visual actor
			if (!actorName)
				actorName = cmpVisual.GetProjectileActor();

			let visualActorLaunchPoint = cmpVisual.GetProjectileLaunchPoint();
			if (visualActorLaunchPoint.length() > 0)
				launchPoint = visualActorLaunchPoint;
		}

So it appears incorrect that this would be a precedent for using actor prop points in simulation state determining data or code (rather the opposite?)

source/simulation2/components/CCmpVisualActor.cpp
692

whitespace

source/simulation2/scripting/EngineScriptConversions.cpp
361

I dont assume that you envision to allow JS to change prop points. If so, specifying this method is wrong.

05:17 < Stan`> elexis actuay I can't because of the macro usage.

One can.

367

FromJSVal means that we create a new JS object without properties and then try to read the name and position property of that empty object?

Stan added a comment.Sep 30 2019, 11:58 AM

I assume this sentence is written because Phabricator refuses empty test fields.

Actually no, I guess I could improve it a bit. Else I would just have written a blank space.

(Ammo points)

In non visual it defaults to (0,0,0) vector at the root of the object.

source/simulation2/components/CCmpVisualActor.cpp
687

Newline

source/simulation2/scripting/EngineScriptConversions.cpp
361

So how does one prevent that when the function is explicitely called by a macro you asked me to use ?

367

I guess not just copied the code above, as I said, I didn't need that function until I started using the macro.

Stan edited the test plan for this revision. (Show Details)Sep 30 2019, 12:02 PM
Stan added a comment.Sep 30 2019, 12:09 PM

Also I'm not sure we can Use sproppoint as it does not contain the correct prop location and will always need to be modified anyway

In D1989#97919, @Stan wrote:

I assume this sentence is written because Phabricator refuses empty test fields.

Actually no, I guess I could improve it a bit. Else I would just have written a blank space.

(Ammo points)

In non visual it defaults to (0,0,0) vector at the root of the object.

But that is the visual point, not the point used by the simulation to determine the trajectory of the projectile, correct?
The simulation must compute the same simulation state in visual and nonvisual mode.

Also specifying simulation dependent data in the actor file means that mod compatibility is broken,
you for instance can't provide a different actor anymore without causing an OOS.
For example a low-polygon mod would have to have the exact same proppoints in the file.

Currently it is not possible to access the model's prop point in the JS code which for instance makes it that you have to manually specify visible garrison prop points, while you could use the ones defined by artists in the mesh.

So it should use the templates like the projectile launch point correct?
Which would mean abandoning this, unless you can show that actor file contents already determine the simulation state and that further breaking actor mod support is worth it.

source/simulation2/scripting/EngineScriptConversions.cpp
361

You are permitted to find a solution that meets the objective of the macro without implementing a function that is not used and depending on design of the patch must or must not exist.

Stan updated this revision to Diff 10023.Sep 30 2019, 2:21 PM
Stan marked an inline comment as done.
  • Adds rotation to the mix, showing that no information is there about the negative Z
  • Hopefully fix the FromJsval() I'm not really confident with those things.

Now to the usefulness of that patch, I'm unfortunately forced to agree with you that it would be a major issue with mod compatibility, though one always have to be careful with models. It's also sad that we can't have artists animated towers, for instance this

we'd have to do it through code, and that after D1958 most of the rider props will be useless.

However there is no way to make it work the same way in visual and non visual (except maybe by specifying both in the template) which defeats the point, I guess.

So I'm gonna abandon this, but keep it to easily get the prop point position when making new turrets.

Stan abandoned this revision.Sep 30 2019, 2:21 PM
Stan added inline comments.
source/simulation2/scripting/EngineScriptConversions.cpp
361

That makes no sense to me, as I would've done it had I know how to :) but thanks I guess.

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/363/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/simulation2/components/ICmpVisual.h
|  47| class·ICmpVisual·:·public·IComponent
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classICmpVisual:' is invalid C code. Use --std or --language to configure the language.

source/simulation2/scripting/EngineScriptConversions.cpp
| 174| »   »   FAIL_VOID("Failed·to·get·Vector3D·constructor");
|    | [MAJOR] CPPCheckBear (unknownMacro):
|    | There is an unknown macro here somewhere. Configuration is required. If STMT is a macro then please configure it.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/879/display/redirect

elexis added inline comments.Sep 30 2019, 6:13 PM
source/simulation2/scripting/EngineScriptConversions.cpp
361

This is an example of how to define only one of the two functions without having to reimplement the vector<->JS array conversion:

class IComponent;
template<> void ScriptInterface::ToJSVal<std::vector<IComponent*> >(JSContext* cx, JS::MutableHandleValue ret, const std::vector<IComponent*>& val)
{
	ToJSVal_vector(cx, ret, val);
}

template<> bool ScriptInterface::FromJSVal<std::vector<Entity> >(JSContext* cx, JS::HandleValue v, std::vector<Entity>& out)
{
	return FromJSVal_vector(cx, v, out);
}