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

Add Lua Scripting for Carryall. #19312

Merged
merged 1 commit into from Jan 12, 2022
Merged

Conversation

MustaphaTR
Copy link
Member

I changed the Carryalls in SP to actually use Carryall logic. The shellmap had some carryall stuff, which we didn't support yet for the actual Carryall logic. This PR adds functions for Picking Up and Dropping the carried unit for Carryalls. Altho i'm not sure if what i did for ignoring the Pickup/Deliver if the Carryall is already Full/Empty is the best way of doing it.

@pchote
Copy link
Member

pchote commented Apr 3, 2021

There are a couple of issues here, demonstrated with a testcase:

  • Add the following to sunstroke.lua:
    WorldLoaded = function()
    	Multi = Player.GetPlayer("Multi0")
    	local carryall = Actor.Create("TRNSPORT", true, { Owner = Multi, Location = Actor615.Location })
    	local cargo = Actor.Create("APC", true, { Owner = Multi, Location = Actor615.Location + CVec.New(3, 3) })
    	carryall.PickupUnit(cargo)
    	carryall.Move(Actor615.Location - CVec.New(3, 3))
    	carryall.DeliverUnit()
    end
  • Start a game at the A spawn
  • Select the carryall and hold shift to watch target lines

The first problem is that this this generates target lines, whereas the standard behaviour for scripted activities (whether we like it or not - this isn't the place to argue it) is to hide them by passing null to a targetLineColor arg in the activity constructors. This will need to be hooked up for PickupUnit and DeliverUnit.

The second problem is that the if (carryall.Carryable == null) check in DeliverUnit runs immediately, before the cargo is loaded, so delivery is skipped. This should be deferred until the activity runs using the CallFunc activity.

Can we please also call the Lua APIs for these PickupCarryable and DropCarryable? "Unit" is too general for the actor namespace and "Deliver" implies it should take a location as an argument. This isn't the first time that we hide the true activity names from the API.

@abcdefg30
Copy link
Member

This should be deferred until the activity runs using the CallFunc activity.

I'm not quite sure what the suggestion here is. Having the checks at all seems unnecessary. (And shouldn't be worked around in Lua code using CallFunc?) The activities should handle that case on their own.

@MustaphaTR
Copy link
Member Author

Updated. Fixed the first and 3rd points as PChote said. I removed the checks as abcdefg30 said, but haven't checked/updated the activities to properly check them. I have some questions first, will handle that along with them.

  1. Should AutoCarryall and FreeActorWithDelivery have target lines? It doesn't matter much for base mods, since our only usecase for those traits are not selectable. But also they are automatic orders, that are not done by the player, similar to lua scripts.
  2. I've just noticed DeliverUnit has a second constructor that has a target arguement. If i change the lua function to have an optional target arguement and use either depending on if it is defined, should the function still be called DropCarryable or is DeliverCarryable better then. Should the arguement be optional at the first place?

@pchote
Copy link
Member

pchote commented Apr 4, 2021

  1. I'm not sure, and I think we risk derailing this PR if we go too far down that topic (I'm personally not convinced about disabling target lines at all)
  2. I think it would be best to have a single DeliverCarryable that always requires a destination.

@MustaphaTR
Copy link
Member Author

Updated with the point 2. Activities already seem to be properly ignoring the Pickup/Deliver order if they are already Full/Empty. You can see with this version of the testcase PChote posted above.

WorldLoaded = function()
	Multi = Player.GetPlayer("Multi0")
	local carryall = Actor.Create("TRNSPORT", true, { Owner = Multi, Location = Actor615.Location })
	local apc = Actor.Create("APC", true, { Owner = Multi, Location = Actor615.Location + CVec.New(3, 3) })
	local bike = Actor.Create("BIKE", true, { Owner = Multi, Location = Actor615.Location + CVec.New(0, 3) })
	carryall.PickupCarryable(apc)
	carryall.PickupCarryable(bike)
	carryall.DeliverCarryable(Actor615.Location - CVec.New(3, 3))
	carryall.DeliverCarryable(Actor615.Location + CVec.New(3, 3))
end

@MustaphaTR
Copy link
Member Author

This has been sitting for a while, i would like to note again that this was updated and the label is inaccurate. I can't edit labels anymore.

@abcdefg30
Copy link
Member

This has compiler errors once you rebase.

@MustaphaTR
Copy link
Member Author

Rebased and updated.

penev92
penev92 previously approved these changes Jan 9, 2022
Copy link
Member

@penev92 penev92 left a comment

Choose a reason for hiding this comment

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

LGTM with one small fix request

OpenRA.Mods.Common/Traits/AutoCarryall.cs Outdated Show resolved Hide resolved
Copy link
Member

@penev92 penev92 left a comment

Choose a reason for hiding this comment

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

👍

@abcdefg30 abcdefg30 merged commit d149624 into OpenRA:bleed Jan 12, 2022
@abcdefg30
Copy link
Member

Changelog

@MustaphaTR MustaphaTR deleted the carryall-lua branch January 12, 2022 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants