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

Made both Cargo and Carryall conditional #20534

Merged
merged 1 commit into from Aug 13, 2023

Conversation

Mailaender
Copy link
Member

Closes #17330

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.

PickupUnit activity will also need this check, otherwise it doesn't cancel properly. I managed to glitch out the transport by giving it a pickup order at the same moment it was loading infantry.

The testcase commit should also be renamed to testcase

OpenRA.Mods.Common/Traits/Cargo.cs Show resolved Hide resolved
@PunkPun
Copy link
Member

PunkPun commented Dec 10, 2022

This should also be tested with the AutoCarryall trait, I imagine there will also need to be checks added to FerryUnit activity, and to the trait itself

@CDVoidwalker
Copy link
Contributor

This should also be tested with the AutoCarryall trait, I imagine there will also need to be checks added to FerryUnit activity, and to the trait itself

Tested that, as expected AutoCarryall ignores conditions, for most cases it can be simply fixed by removing actors with disabled AutoCarryall from being requested to pickup Carryables.

diff --git a/OpenRA.Mods.Common/Traits/AutoCarryable.cs b/OpenRA.Mods.Common/Traits/AutoCarryable.cs
index c248d7146b..cbeccc6583 100644
--- a/OpenRA.Mods.Common/Traits/AutoCarryable.cs
+++ b/OpenRA.Mods.Common/Traits/AutoCarryable.cs
@@ -64,7 +64,7 @@ void RequestTransport(Actor self, CPos destination)
 
 			// Inform all idle carriers
 			var carriers = self.World.ActorsWithTrait<Carryall>()
-				.Where(c => c.Trait.State == Carryall.CarryallState.Idle && !c.Actor.IsDead && c.Actor.Owner == self.Owner && c.Actor.IsInWorld)
+				.Where(c => c.Trait.State == Carryall.CarryallState.Idle && !c.Trait.IsTraitDisabled && !c.Actor.IsDead && c.Actor.Owner == self.Owner && c.Actor.IsInWorld)
 				.OrderBy(p => (self.Location - p.Actor.Location).LengthSquared);

However if the AutoCarryall gets disabled mid carrying it will still ignore the condition. That could be fixed depending on desired behaviour in that scenario. Should the carryall attempt to deliver the cargo and wait untill the condition has expired or just stand in place being unable to perform its task?

@Mailaender
Copy link
Member Author

Adjusted AutoCarryable.

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.

A disabled check should be added to FindCarryableForTransport in AutoCarryall.

Also, other issues mentioned by me are still unresolved

@PunkPun
Copy link
Member

PunkPun commented Jan 23, 2023

PickupUnit and AttachUnit activities still need IsTraitDisabled checks

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.

  • FerryUnit activity still needs to cancel OnFirstRun when carryall is disabled
  • RideTransport activity needs to cancel on carryall becoming disabled
  • AutoCaryall needs IsTraitDisabled checks in FindCarryableForTransport and RequestTransportNotify

OpenRA.Mods.Common/Activities/PickupUnit.cs Outdated Show resolved Hide resolved
@PunkPun
Copy link
Member

PunkPun commented Jan 26, 2023

D2K testcase
Add to outpost

ProvidesPrerequisite@radar:
	Prerequisite: radar
	RequiresCondition: !disabled

Add to carryall

AutoCarryall:
	RequiresCondition: enabled
GrantConditionOnPrerequisite:
	Condition: enabled
	Prerequisites: radar

this allows you to toggle all caryalls

@PunkPun
Copy link
Member

PunkPun commented Jan 28, 2023

Works almost perfectly. Only one issue left, when AutoCaryall is carrying a unit and AutoCaryall gets disabled, the unit gets deleted. Though if landing was initiated it gets placed down fine.

I guess this was fixed in #20505 on prep and will be fixed in #20490 on bleed

PunkPun
PunkPun previously approved these changes Jan 28, 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
Copy link
Member

PunkPun commented Jul 25, 2023

needs a rebase

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.

It's a bit weird when AutoCarryall is disabled when carrying a unit. It gets stuck carrying that unit forever.

I can see 2 main solutions, 1st is to immediately drop of the Carryable once AutoCarryall becomes disabled, or the other is to trigger a drop off once Autocarryall becomes enabled and has cargo. Personally I don't feel that it's a must to address in this PR

OpenRA.Mods.Common/Traits/Passenger.cs Show resolved Hide resolved
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.

RideTransport should have an TickInner override, which should contain an IsTraitDisabled and call Cancel when Cargo becomes disabled. Otherwise units can be chasing the the disabled cargo forever.

Might as well also include a check wether the unit is even valid to be carries, as in is there enough space for it? This would make loading up transports much easier

@Mailaender Mailaender force-pushed the conditional-cargo-carryall branch 2 times, most recently from 544eff2 to 4cce8d5 Compare August 13, 2023 15:18
@PunkPun PunkPun merged commit 98896f9 into OpenRA:bleed Aug 13, 2023
3 checks passed
@PunkPun
Copy link
Member

PunkPun commented Aug 13, 2023

Changelog

@Mailaender Mailaender deleted the conditional-cargo-carryall branch August 13, 2023 16:45
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.

Actors cannot have multiple Cargo: traits
3 participants