Add deploy while moving to Saboteur#21661
Conversation
PunkPun
force-pushed
the
deploy
branch
2 times, most recently
from
c9128f5 to
41007ed
Compare
| public readonly bool CanCancelCondition = false; | ||
|
|
||
| [Desc("Should we interrupt the current activity and wait for it to finish?")] | ||
| public readonly bool CancelsCurrentOrders = false; |
There was a problem hiding this comment.
What is the use case of this flag? We have GrantConditionOnDeploy for things that "deploy" (i.e. cancel current orders). It would be better to keep the "OnDeploy" terminology for that use case and keep this trait separate.
There was a problem hiding this comment.
grant condition on deploy doesn't do charge, the usecase is the general flexibility conditions allow to modders. The condition may trigger something that needs the unit to be stationary
There was a problem hiding this comment.
Charge-vs-not and Deploy-vs-activate are orthogonal features. The timed-condition-on-deploy feature should be either a feature of GrantConditionOnDeploy or a separate GrantChargedConditionOnDeploy trait.
| [Desc("Allow deploying on specified charge to grant a condition for a specified duration.")] | ||
| public class GrantConditionOnDeployWithChargeInfo : PausableConditionalTraitInfo, IRulesetLoaded | ||
| [Desc("Grant a condition via player orders for a specified amount of time.")] | ||
| public class GrantChargedConditionOnToggleInfo : PausableConditionalTraitInfo, IRulesetLoaded |
There was a problem hiding this comment.
This uses "Toggle" but most of the fields below use "Activat(ed|ion)". Can we standardise on Activation if this must have a suffix? (I still don't think it needs one.)
There was a problem hiding this comment.
we do use the name toggle for toggle actions
PunkPun
force-pushed
the
deploy
branch
2 times, most recently
from
1f6f506 to
ac4d6a7
Compare
| [Desc("Can " + nameof(ActivatedCondition) + " be canceled turned off manually?")] | ||
| public readonly bool CanCancelCondition = false; | ||
|
|
||
| [Desc("Should we interrupt the current activity")] |
There was a problem hiding this comment.
"... if not queued" is an important bit of information here.
There was a problem hiding this comment.
it makes it more confusing, in the same way my addition was
Also allow deploying without cancelling current activity, and make saboteurs use it
penev92
left a comment
penev92
left a comment
There was a problem hiding this comment.
The naming disagreements aside, thanks for the fix.
|
The commit message has a typo. |
|
It's a merged PR |
4 participants
Enabling cloak should not be interfering with the current actors activity