-
-
Notifications
You must be signed in to change notification settings - Fork 105
Unit & infantry auto-conversion on ammo change #1653
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
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. |
Think it's still better to batch these conditional conversion and AutoDeath in a same condition group, so it has the following benefits:
see #1346 |
Two concerns:
|
|
We can re-think the code after the merge into develop of #1346 but for now I need this feature for my mod and I share the current implementation :-D |
There is not much difference indeed. I think you just need to additionally explain the actual check process in the document. |
You don't need to worry about ConvertToType. If the conversion can not be processed, it will exit safely. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Only one small concern.
src/Ext/Techno/Body.Update.cpp
Outdated
const auto pFoot = abstract_cast<FootClass*>(pThis); | ||
const auto pFoot = pThis && (pThis->AbstractFlags & AbstractFlags::Foot) ? static_cast<FootClass*>(pThis) : nullptr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const auto pFoot = abstract_cast<FootClass*
, true >(pThis);
is a method of using encapsulated functions, which can be directly used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But you are suggesting a code change or simply explaining a recommendation for future similar situations? is for going direct & closing the pending conversation :-P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to do sanity check on pThis
. You have already used pThis->Ammo
and are even still using pThis->GetTechnoType()
, it cannot be empty.
I explained for a while just to make you understand what the true
in abstract_cast
means.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok now I understood, so I'll leave as
const auto pFoot = abstract_cast<FootClass*, true>(pThis);
because as you said pThis was already used and the last bit of information i wanted to know is if pThis is a FootClass object or something else for future things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if pThis is a FootClass object or something else for future things.
Sorry, I didn't understand what you meant. pThis
is a TechnoClass
pointer and the converted pFoot
is a FootClass
pointer. Unless it is not a Foot
(such as a Building
), they all point to the same entity.
pThis here will never be nullptr so we skip this check.
Ammo.AutoConvertMinimumAmount
determines the minimal number of ammo at which a unit converts automatically.Ammo.AutoConvertMaximumAmount
determines the maximum number of ammo at which a unit converts automatically.Ammo.AutoConvertType
specify the new techno after the conversion. This unit must be of the same type of the original (vehicle -> vehicle or infantry -> infantry).In
rulesmd.ini
: