Page MenuHomeWildfire Games

Move identity classes to the mirage filter
AbandonedPublic

Authored by temple on Nov 28 2017, 8:56 PM.

Details

Reviewers
None
Summary

In D750/rP19987 identity was added to the mirage component so it had access to the classes list. However, we remove the classes list in the mirage filter, which seems pointless if we just add it back later. Here's the comment from rP15612 explaining why the classes list wasn't included in the mirage:

Select a subset of identity data. We don't want to have, for example, a CC mirage
that has also the CC class and then prevents construction of other CCs

However, the mirage filter doesn't include BuildRestrictions so it's not looked at in the distance check (of BuildRestrictions) and it doesn't change the entity counts (of EntityLimits), and it does include Foundation so it doesn't change the entity limits (of EntityLimits). So already this doesn't happen. (It would've been broken by D750/rP19987 if it had.)

Test Plan

Are there examples where we wouldn't want some part of the identity component in the mirage?

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

temple created this revision.Nov 28 2017, 8:56 PM
bb added a comment.Nov 29 2017, 12:20 PM

But now it does change the entitycounts in enititylimits, right? warning the enity id in the onGlobalOwnershipchanged in enitylimits, reveals the function is called, but just currently the classlist is empty, so nothing gets updated. Now that there is a classlist, the things will get updated, thus break. I guess mirages should be excluded from this. Perhaps the same goes for the conquest checks (IIrc one sometimes had to explore some stuff and then won, I guess that is due to mirages...)
This wasn't broken before, since there we call engine.QueryInterface, which doesn't have the classes, while QueryMiragedInterface returns the mirage component and does have the classes.

Maybe there are more places where this breaks (statisticsTracker is a good candidate)

temple abandoned this revision.Nov 29 2017, 3:00 PM
In D1078#42988, @bb wrote:

But now it does change the entitycounts in enititylimits, right? warning the enity id in the onGlobalOwnershipchanged in enitylimits, reveals the function is called, but just currently the classlist is empty, so nothing gets updated. Now that there is a classlist, the things will get updated, thus break.

No, as I said it doesn't change the counts because mirages don't have BuildRestrictions.

This wasn't broken before, since there we call engine.QueryInterface, which doesn't have the classes, while QueryMiragedInterface returns the mirage component and does have the classes.

Ah, okay. I added that parenthetical as an afterthought but I should've examined it more closely.

Maybe there are more places where this breaks (statisticsTracker is a good candidate)

This seems to be working fine too.

But as there's lots of places where this could potentially break things it's probably better not to do it, at least without a better reason. So I'll abandon.

bb added a comment.Nov 30 2017, 1:52 PM

There are arguments either way, One could say that depending on the fact that classes are not in the mirage identity component implies that some code isn't executed is bad practise. On the other hand a mirage is a ghost sorta, so how could it have classes?

Thus abolish seems fine, until it bites us harder :P

elexis added a subscriber: elexis.Mar 6 2018, 5:32 PM

Perhaps the same goes for the conquest checks (IIrc one sometimes had to explore some stuff and then won, I guess that is due to mirages...)

Whut? I hope that's not the case or that it's reported if it is. I suspect that it isn't.
I can't imagine this to be true however. A player has it's owned entities always visible, so it's defeat doesn't depend on visibility. Since it's the EndGameManager setting the winner if everyone else is defeated, the victory doesn't depend on visibility either, right?

There are arguments either way, One could say that depending on the fact that classes are not in the mirage identity component implies that some code isn't executed is bad practise. On the other hand a mirage is a ghost sorta, so how could it have classes?

How do you mean? The mirage entity needs classes so that we can determine at least the attack cursor, so it must be in there? And the classeslist is read from, so it's not unused code?

Maybe there are more places where this breaks (statisticsTracker is a good candidate)
But as there's lots of places where this could potentially break

You mean the attack cursor fix could have broken other things we didn't notice yet by adding the classeslist without investigating all places that use the classeslist?