Page MenuHomeWildfire Games

Fix VisualActor tech changes not being applied in some cases
Needs ReviewPublic

Authored by wraitii on May 29 2017, 11:02 PM.

Details

Reviewers
leper
Sandarac
Trac Tickets
#2907
Summary

Visual actors can be modified by technologies, but the following scenarios don't work:

  • Entities created after the modification-message don't have it
  • Fogged entities don't have it, nor when initially fogging, after a defogging, and if a player upgrades a fogged entity and the tech has no classes, it will show the change (really this is just broken in 99% of cases here).
  • Corpses don't have it
  • Building placement preview don't have it
  • Building foundation previews don't have it.

This fixes that.

Test Plan

Use the dummy 'test.json' tech, from the CC, to test things out.

I think the scenarios above are exhaustive.

Event Timeline

Sandarac created this revision.May 29 2017, 11:02 PM
Vulcan added a subscriber: Vulcan.May 30 2017, 2:02 AM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

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

leper requested changes to this revision.Jun 3 2017, 7:04 PM

I'm not sure if it is worth extra logic to handle the case where an actor changing technology is researched while the user is placing a building.

This does not seem to be handled, or is it?

binaries/data/mods/public/simulation/components/GuiInterface.js
1177 ↗(On Diff #2312)

Should this be adjusted too? (Note that I haven't tried the patch yet.)

source/simulation2/components/CCmpVisualActor.cpp
258

Now I do wonder why that comment is here, given that we do serialize m_ActorName in SerializeCommon. Care to investigate?

537

This needs to be done regardless of whether m_Unit is valid or not, otherwise we will have differences when trying to do a non-visual replay or a serialization test (both of which will result in m_Unit not being valid.

This revision now requires changes to proceed.Jun 3 2017, 7:04 PM
Sandarac updated this revision to Diff 2429.Jun 4 2017, 7:08 PM
Sandarac edited edge metadata.

Update SetWallPlacementPreview, remove uneeded QueryOwnerInterface call in GuiInterface.js (just use the player var already passed to the functions), remove old comment in cmpVisual, remove the return in SetActor.

Sandarac marked 3 inline comments as done.Jun 4 2017, 7:35 PM
In D576#24598, @leper wrote:

I'm not sure if it is worth extra logic to handle the case where an actor changing technology is researched while the user is placing a building.

This does not seem to be handled, or is it?

It is handled (but it wasn't for wall placement, as you pointed out - that's been added). It seems placement preview entities are updated correctly when a tech is researched, but the call to ApplyValueModificationsToTemplate is needed for placing new entities which are affected by the tech.

When placing a building when a tech is researched using the attached JSON in the Test Plan, but replacing "requirements": {"civ": "maur"}, with
"requirements": {"all": [{"tech": "phase_city"}, {"civ": "maur"}]}, (and changing the changephase cheat to "t"):

binaries/data/mods/public/simulation/components/GuiInterface.js
1177 ↗(On Diff #2312)

Yes, you're right, it should have been handled, done now.

source/simulation2/components/CCmpVisualActor.cpp
258

I took a look at the history, and it seems this comment simply wasn't removed/updated when visual actor tech changes were added in rP14928/#2243 (which added the actor name serialization), so I guess it's okay to remove it.

537

Okay.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

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

leper requested changes to this revision.Jul 9 2017, 1:09 AM

Foundation.js needs another addition, beware that you will need to add <Identity merge=""/> to special_filter/construction.xml if you want that to be affected by the technology.

Add

let visualActor = ApplyValueModificationsToTemplate("VisualActor/Actor", cmpPreviewVisual.GetActorName(), this.owner, this.finalTemplateName);
cmpPreviewVisual.SetActor(visualActor);

around line 250. It does seem like a building that was in construction while such a tech was researched will not update the construction preview (construction|) will not have it's construction| actor swapped.

Apart from that it seems to work nicely.

This revision now requires changes to proceed.Jul 9 2017, 1:09 AM
wraitii commandeered this revision.Jun 2 2020, 10:39 PM
wraitii added a reviewer: Sandarac.
wraitii updated this revision to Diff 12106.Jun 2 2020, 10:49 PM
wraitii marked 3 inline comments as done.
wraitii edited the summary of this revision. (Show Details)
wraitii edited the test plan for this revision. (Show Details)

(commandeering, this isn't just a rebase).

It seems that we had a larger bug: entities created after the "valueModification" message would never receive it, since we don't check for value modifications at init.

This fixes that (correctly with regards to serialisation) by changing the actor name on OwnershipChanged.
This fixes most of the trouble with the original patch, since it means foundation preview, corpses and to some extent fogged entities work out of the box.

Foundation previews, however, do not currently have identity classes, so transformation fails for them. This adds it, since that seems innocuous enough.

Mirages also do not have identity classes (see discussion at D1325 and D1078 following D215). The reason appears to be that doing that requires changing a lot of code to make sure things still work, and it seems to be setup as a "pit of failure" where someone will make the same mistake in the future. I see the point of the reasoning.
However, this breaks modifications for mirages, which is rarely an obvious problem, but is there. D274 changed that code, but the bug was existing in earlier code.
I fix it here by querying the mirages interface explicitly, which is slightly slower for all entities, but the cost should not be too large compared to the rest of this function and it's definitely more correct™.

This still isn't enough -> Mirages entities get ValueModification messages, so they would update in FOW. We can't simply use Mirage.js to copy the data, since C++ components have no convenient way of accessing that data (one approach would be to add a function for any variable we might request, which seems unscalable). The simplest solution is to ignore those messages for mirages. There doesn't seem to be other faulty C++ code in that regard.

With that, we need one final fix: mirage aren't deleted when they become visible again, they're merely fogged, and so we need to force the visual actor to reload when the actor is fogged again, or it won't 'catch up' to new modifications.
I'm currently using a hack to do this, I should probably addd a proper function.


In conclusion, I laugh at the "simple" tag in #2907 .

Owners added a subscriber: Restricted Owners Package.Jun 2 2020, 10:49 PM
wraitii added inline comments.Jun 2 2020, 10:53 PM
binaries/data/mods/public/simulation/templates/special/filter/construction.xml
4

One might be concerned that this will change EntityLimits, but as written in #2907, global subscriptions don't receive update for local entities:

// Special case: Messages for local entities shouldn't be sent to script
// components that subscribed globally, so that we don't have to worry about
// them accidentally picking up non-network-synchronised data.

So entity limits can't be changed from local entities such as a foundation preview

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

Linter detected issues:
Executing section Source...

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

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

wraitii added inline comments.Jun 2 2020, 10:55 PM
binaries/data/mods/public/simulation/components/Mirage.js
74

Reworded as I found the original explanation rather unclear.

source/simulation2/components/CCmpVisualActor.cpp
313

debug code left in, need to delete this obviously

Stan added a subscriber: Stan.Jun 3 2020, 8:50 AM
Stan added inline comments.
binaries/data/mods/public/simulation/components/Fogging.js
116

What would be the proper fix?

119

do we need to invalidate both? Below only check for one or the other.

binaries/data/mods/public/simulation/templates/special/filter/construction.xml
4

I think there was some comment about that assumption in D2704

source/simulation2/components/CCmpVisualActor.cpp
258

@wraitii still accurate?

298

Isn't that needed for nonvisual?

309

ternary?

320

Can this be another patch? Should be mostly straightforward.

wraitii added inline comments.
binaries/data/mods/public/simulation/components/Fogging.js
116

I need to add a "RefreshActorName" function or something. Will do it, but didn't bother yesterday evening.

binaries/data/mods/public/simulation/templates/special/filter/construction.xml
4

good point, thanks

source/simulation2/components/CCmpVisualActor.cpp
298

I need to move this below as actor name is serialised, and this changes serialised actor names.

I might have to still use the check below though, I haven't actually checked if this worked in non-visual and it probably doesn't.

309

Meh, line too long for that to really improve readability.

320

I mean it can, but it also makes sense as part of this patch, doesn't it?

Stan added inline comments.Jun 3 2020, 9:00 AM
source/simulation2/components/CCmpVisualActor.cpp
320

I guess, but if it's an optimization, it could be profiled in it's own patch :D

wraitii added inline comments.Jun 3 2020, 9:05 AM
source/simulation2/components/CCmpVisualActor.cpp
320

It's not an optimisation, it's a bug fix.

Otherwise, a fogged entity that receives a ValueModification will update its actor, so you can actually see in FOW that the actor changed, which is not correct.

wraitii updated this revision to Diff 12112.Jun 3 2020, 1:40 PM

Fix issues & hack.
Ran a rejointest, & visual/non visual-replays, all appears in order

Vulcan added a comment.Jun 3 2020, 1:54 PM

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

Linter detected issues:
Executing section Source...

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

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

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

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

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