[Minor] Update TechnoType's loading#2062
[Minor] Update TechnoType's loading#2062Coronia wants to merge 1 commit intoPhobos-developers:developfrom
Conversation
Coronia
added
Minor
|
Nightly build for this pull request:
This comment is automatic and is meant to allow guests to get latest nightly builds for this pull request without registering. It is updated on every successful build. |
TaranDahl
left a comment
TaranDahl
left a comment
There was a problem hiding this comment.
I'm not sure if it's really necessary. I think the performance of LoadFromINI is irrelevant.
Some functions may be adapted to other types in the future. For example, Kerbiter is developing the OpenTopped for buildings. This will cause inconvenience for these future developments.
In addition, for those flags that support multiple types, such as NoQueueUpToEnter, devs still need to write read operations in more places.
| this->Passengers_SyncOwner_RevertOnExit.Read(exINI, pSection, "Passengers.SyncOwner.RevertOnExit"); | ||
| this->Explodes_KillPassengers.Read(exINI, pSection, "Explodes.KillPassengers"); | ||
| this->Ammo_Shared.Read(exINI, pSection, "Ammo.Shared"); | ||
| this->Ammo_Shared_Group.Read(exINI, pSection, "Ammo.Shared.Group"); |
There was a problem hiding this comment.
This should work for both infantry and unit.
|
The main point for this is to make it less coupled than performance increment. As of now these tags are a bit clustering and it would be better to keep their job clear the ideal solution for this might be extending Building/Infantry/Unit/AircraftTypeClass and making them inherit TechnoTypeExt, but I guess that's too much of a revamp to do at this point |
|
@Metadorius Come to think of it, why didn't we make Ext inherit like a base class in the beginning? |
|
Because when we started, we only had Ares architecture and had not much idea of anything else. And yes, I do think that making a proper hierarchy is something that needs to be done. ExtData needs to inherit each other, and Ext should inherit the class it's extending I think (without new virtuals or fields). |
TaranDahl
left a comment
TaranDahl
left a comment
There was a problem hiding this comment.
The main point for this is to make it less coupled than performance increment.
Most of the tags work independently. I don't think there is any coupling between them. Are there any examples?
As of now these tags are a bit clustering and it would be better to keep their job clear
It might make sense, but I'm not sure if it makes enough sense to justify the additional effort of cleaning up.
I think it's sufficient to place those flags that act on a single type under the corresponding RTTI. Also, we should consider whether it's valuable to expand to multiple types in the future.
| this->DeployedSecondaryFireFLH.Read(exArtINI, pArtSection, "DeployedSecondaryFireFLH"); | ||
|
|
||
| if (!this->Harvester_Counted.isset() && pThis->Enslaves) | ||
| this->Harvester_Counted = true; |
There was a problem hiding this comment.
Should be under building and unit I think
| this->Wake_Grapple.Read(exINI, pSection, "Wake.Grapple"); | ||
| this->Wake_Sinking.Read(exINI, pSection, "Wake.Sinking"); | ||
|
|
||
| exINI.ReadSpeed(pSection, "SubterraneanSpeed", &this->SubterraneanSpeed); |
There was a problem hiding this comment.
I don't think this is a type-restricted thing, at most it should be restricted to footclass.
3 participants
put many tags into the existing
LoadFromINIByWhatAmIfunction to make it won't read a tag that's incapable to use. If there're more tags that could be put in, or shouldn't be put in certain types, please point it out