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.
Details
- Reviewers
wraitii
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
- Repository
- rP 0 A.D. Public Repository
- Lint
Lint Skipped - Unit
Unit Tests Skipped - Build Status
Buildable 9589 Build 16050: Vulcan Build Jenkins Build 16049: Vulcan Build (Windows) Jenkins
Event Timeline
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.
@Freagarach that's strange, can you try to figure out what was null and shouldn't have been ?
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?
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 | ||
---|---|---|
90 | /** * Can be used for ... */ | |
224 | Why does the simulation store printable names intead of the names? | |
232 | 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. | |
805 | 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. | |
91 | It's not necessary to use the Fixed type for JS. | |
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) |
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 | ||
---|---|---|
90 | /** * Can be used to retrieve special locations given by the artists * to place actors with precision on this actor. */ | |
224 | That's the only function I found that returns a std::string. | |
232 | So something like m_PropPointsPositions.resize(props.size())
Do you have an example of the latter ? | |
805 | 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. | |
91 | In this case since it will affect simulation it needs to fixed. | |
source/simulation2/scripting/EngineScriptConversions.cpp | ||
369 | What are the advantages / disadvantages ? |
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.
- 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
source/simulation2/scripting/EngineScriptConversions.cpp | ||
---|---|---|
369 | Nevermind. Got it ^^' |
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
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
- 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
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
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 | ||
---|---|---|
689 | 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.
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? |
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 | ||
---|---|---|
684 | 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. |
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
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. |
- 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.
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
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); } |