Skip to content
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

AutoCarryall can receive manual order and auto action can be turn on/off by condition #20398

Merged
merged 1 commit into from Aug 18, 2023

Conversation

dnqbob
Copy link
Contributor

@dnqbob dnqbob commented Oct 25, 2022

Summary

carryall-esc-showcase
(1). Autocarryall can receive manual order, make a controllable carryall (unlike the one in D2k) with AutoCarryall easier to manipulate for player.

carryall2-showcase
(2). Autocarryall's auto action can be turn on/off by condition, make (1) even easier to manipulate. In test case, you can use "Deploy" to disable and enable the auto action of TS carryall (when they has no passenger).

The change here is mainly to make AutoCarryall work like AutoTarget-- even if actor can automatically find their own target, player can still order them to attack elsewhere, and you can change to "HoldFire" for a better micromanage on you own.

Side effect

No side effect currently. The carryall in D2K cannot be selected so they are just like before, unless you find bugs or some unexpected behaviors due to this PR.

Some thoughts

I think we can change D2K carryall into my testcase and make it selectable, they are hard to use currently with full auto action.

@dnqbob dnqbob marked this pull request as draft October 25, 2022 15:04
@dnqbob dnqbob marked this pull request as ready for review October 25, 2022 15:33
@dnqbob
Copy link
Contributor Author

dnqbob commented Oct 26, 2022

Plus: I think when there is a high possibility that the creator of Carryall wants to cover some function of AutoCarryall and they messed up until now. I clear some of them but don't know if it is the correct way.

Plus 2: AutoCarryall cannot carry AutoCarryable manually (only allow automatically), is it a bug or feature? I consider it a bug in this PR and fix it.

@abcdefg30
Copy link
Member

Plus 2: AutoCarryall cannot carry AutoCarryable manually (only allow automatically), is it a bug or feature? I consider it a bug in this PR and fix it.

AutoCarryall has auto in its name, why should it carry things manually? What is the problem with making Carryall and AutoCarryall conditionable and enabling/disabling those two traits depending on which behaviour is required?

@dnqbob
Copy link
Contributor Author

dnqbob commented Oct 26, 2022

AutoCarryall has auto in its name, why should it carry things manually? What is the problem with making Carryall and AutoCarryall conditionable and enabling/disabling those two traits depending on which behaviour is required?

Because in our mod (third-party) we require that when switch to AutoCarryall, it can still be ordered by player to carry things if required, not just stay still and refuse to go, which makes our players feel strange about it. They would prefer AutoCarryall works like AutoTarget in this case -- even if actor can automatically find their own target, player can still order them to attack elsewhere.

@dnqbob
Copy link
Contributor Author

dnqbob commented Oct 26, 2022

So I concern that if it is due to somewhere else in OpenRA need AutoCarryall to be like this. This PR does not affect the D2k Carryall, though, for this actor RejectsOrders cannot be selected.

@dnqbob
Copy link
Contributor Author

dnqbob commented Oct 26, 2022

carryall-esc-showcase
Our players mentioned an usecase on this: if harvester get ambushed right on the place to harvest (like there is creeps/sandworm passes by) with a carryall, the most user-friendly way is that you can manually order carryall to pick up the harvester and make a run of it, instead of switching to normal Carryall first.

AutoCarryall in present version of OpenRA doesn't support it, AutoCarryall cannot pick up passenger when player manually order it.

@dnqbob dnqbob changed the title Auto carry action in AutoCarryall can be controlled by condition AutoCarryall can receive manual order and auto action can be turn on/off by condition Oct 26, 2022
@dnqbob
Copy link
Contributor Author

dnqbob commented Oct 26, 2022

What is the problem with making Carryall and AutoCarryall conditionable and enabling/disabling those two traits depending on which behaviour is required?

In addition, we cannot have multiple Carryall trait due to this:
Screenshot_20221027_010229_com huawei browser_edit_27940079116569

AutoCarryall is also a Carryall trait.

Making the Carryall from an unconditional trait to a conditional trait will lead to some problem

  1. how to dealing with reserved or locked carryable actor when the Carryall trait is disabled.
  2. how to deal with already carried actor which tied to a disabled Carryall trait.

In conclusion, making Carryall conditional will result in a refactoring with nasty edge cases.

@abcdefg30
Copy link
Member

Because in our mod (third-party) we require that when switch to AutoCarryall, it can still be ordered by player to carry things if required, not just stay still and refuse to go, which makes our players feel strange about it. They would prefer AutoCarryall works like AutoTarget in this case -- even if actor can automatically find their own target, player can still order them to attack elsewhere.

Ok, that makes sense to me. I expected a switch to entail the carryall landing and dropping off cargo, but this is more meant as controllable unit that usually does its own thing (just like harvesters automatically search for resources but can still be ordered).

@dnqbob dnqbob force-pushed the acarryall-switch branch 3 times, most recently from c8a9f32 to 6ef3c23 Compare November 6, 2022 12:51
@PunkPun
Copy link
Member

PunkPun commented Nov 17, 2022

When you give a pickup order to a carryall while it is moving to pickup the harvester, it doesn't cancel the activity queue. The unit you ordered to pick up will be moved to the place where the carryall was moving in the first place and dropped of there

Screen.Recording.2022-11-17.at.18.47.19.mov

@dnqbob
Copy link
Contributor Author

dnqbob commented Nov 17, 2022

When you give a pickup order to a carryall while it is moving to pickup the harvester, it doesn't cancel the activity queue. The unit you ordered to pick up will be moved to the place where the carryall was moving in the first place and dropped of there

Yes, so we cancel the activity queued?

Although we can use a stop command on this situation

@dnqbob
Copy link
Contributor Author

dnqbob commented Dec 12, 2022

Should be fixed now.

@dnqbob dnqbob force-pushed the acarryall-switch branch 2 times, most recently from 4ad2fb0 to 75b39fd Compare December 12, 2022 03:57
@PunkPun
Copy link
Member

PunkPun commented Dec 12, 2022

This PR now clashes with #20534 and #20490. I'm thinking it would be worth waiting for those to be merged before going forward with this

@dnqbob
Copy link
Contributor Author

dnqbob commented Dec 12, 2022

you can merge those PRs first. I will handle the conflicts.

OpenRA.Mods.Common/Traits/AutoCarryall.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/AutoCarryall.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/AutoCarryall.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/AutoCarryall.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/AutoCarryall.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/AutoCarryable.cs Outdated Show resolved Hide resolved
PunkPun
PunkPun previously approved these changes Aug 18, 2023
Copy link
Member

@PunkPun PunkPun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@PunkPun PunkPun merged commit c9dfb21 into OpenRA:bleed Aug 18, 2023
3 checks passed
@PunkPun
Copy link
Member

PunkPun commented Aug 18, 2023

Changelog

@dnqbob dnqbob deleted the acarryall-switch branch August 18, 2023 22:03
@Porenutak
Copy link
Contributor

Porenutak commented Feb 5, 2024

Summary

carryall-esc-showcase carryall-esc-showcase

@dnqbob pls can you post yaml from example above? Im trying to recreate AUTO/MANUAL decoration from your example but I stuck.

heres my yaml that dont work:
controlableCarryall.yaml.txt

@dnqbob
Copy link
Contributor Author

dnqbob commented Feb 29, 2024

@Porenutak dnqbob@729832d

Noted that due to some recently change you cannot use mouse click to deploy because GrantConditionOnDeploy conflict with Unload, but you can try pressing [F].

I think maybe add a autotarget and using stance will be a better idea,
or you can code a new GrantConditionOnDeploy that override carryall's Unload only when carryall has no cargo.

@Porenutak
Copy link
Contributor

Or probably if AutoCarryall grant condition to itself if its on.
The point is that player must know if the carryall is on auto mode or not. Its not obvious if you just looks at it. Definitly not in heat of battle.

@dnqbob
Copy link
Contributor Author

dnqbob commented Feb 29, 2024

Yep, so the problem is the image is not showing? I suggest to use WithTextDecoration in my example.

@Porenutak
Copy link
Contributor

But how to trigger WithTextDecoration with condition when GrantConditionOnDeploy dont work? Is there any other way?

@PunkPun
Copy link
Member

PunkPun commented Feb 29, 2024

stances use conditions afaik

@dnqbob
Copy link
Contributor Author

dnqbob commented Feb 29, 2024

But how to trigger WithTextDecoration with condition when GrantConditionOnDeploy dont work? Is there any other way?

My example code works.
1

@dnqbob
Copy link
Contributor Author

dnqbob commented Feb 29, 2024

I don't see any problem in your code, could you try use my example code in your mod?

Is there any other way?

You can use stances in AutoTarget, which will requre you to bind a fake weapon, or maybe we can fix this deploy key override issue @PunkPun.

@Porenutak
Copy link
Contributor

Porenutak commented Feb 29, 2024

If I select carryall and for example order it to move elsewhere it go there and stay idle. That mean carryall switch from auto mode to manual because he no longer automaticly transports Harvesters or anything else. But the Auto text is still visible. This will confuse people. Because they will think carryall is on auto mode but its not, it just flying idle with harvester.

2024-02-29.13-35-02.mp4

@dnqbob
Copy link
Contributor Author

dnqbob commented Feb 29, 2024

Maybe add

	RejectsOrder@AutoCarryall:
		RequiresCondition: !manual
		Except: GrantConditionOnDeploy ## Use 'SetUnitStance' if you use AutoTarget
		RemoveOrders: true

which will make AutoCarryall only recieve GrantConditionOnDeploy/SetUnitStance order when at auto mode, rejects all other orders.

If you want text is compeletely sync with current carryall behavior.

@Porenutak
Copy link
Contributor

thx, this work, but overide issue with GrantConditionOnDeploy still remain.

Use 'SetUnitStance' if you use AutoTarget

Im missing something. How are stances related to this issue?
Also cant found any info for SetUnitStance in dokumentation or in autotarget trait

@dnqbob
Copy link
Contributor Author

dnqbob commented Feb 29, 2024

Im missing something. How are stances related to this issue?

Well becuase of overide issue with GrantConditionOnDeploy, if you need an alternative way to on switching AutoCarryall you can try add AutoTarget to the actor and player will use
image
to turn on/off the AutoCarryall. For example, you can add:

AutoTarget:
		HoldFireCondition: manual
		ReturnFireCondition: manual
		InitialStance: HoldFire

So switch carryall to HoldFire or ReturnFire in this case will enable manual mode, otherwise automatic mode.

@dnqbob
Copy link
Contributor Author

dnqbob commented Feb 29, 2024

Also cant found any info for SetUnitStance in dokumentation or in autotarget trait

See line 213 in AutoTarget.cs. If you really going to use AutoTarget then you should RejectsOrder Except the order 'SetUnitStance', or you can never set the stance in automatic mode.

@dnqbob
Copy link
Contributor Author

dnqbob commented Feb 29, 2024

Compared with GrantConditionOnDeploy, using AutoTarget is very hacky but solve the problem of collision between unload order and deploy order.

@Porenutak
Copy link
Contributor

Thanks for advice, I will try it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants