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 a CanDeploy check to Transform at FrameEndTask #20527

Merged
merged 1 commit into from
Mar 11, 2023

Conversation

PunkPun
Copy link
Member

@PunkPun PunkPun commented Dec 7, 2022

Closes #13984

abcdefg30
abcdefg30 previously approved these changes Dec 8, 2022
@anvilvapre
Copy link
Contributor

won't it still show the undeploy animation? or is that no issue? it will just not deploy at the end?

@PunkPun
Copy link
Member Author

PunkPun commented Dec 9, 2022

Oh I just realised. MCV doesn't have a deploy animation so actually I'm unsure. The animation might play but the unit won't transform.

We can't do much about this, it's a choice. Do we want the transform always after the animation stops playing or only if it's viable?

I see 3 options

  • Only transform when viable
  • If there's an animation - don't check, if there's no animation - check
  • Expose this to yaml and let the modder decide on what should happen upon the animation completing

Skipping the check would be as simple as passing a null into the DoTransforms() so on a technical level each option is a clean solution. Though customisation might be a bit messy considering so many traits use this activity. FWIW I'm fine with the current state of the PR

@PunkPun
Copy link
Member Author

PunkPun commented Dec 9, 2022

I made it so when after playing the make animation deploying is invalid, it plays it in reverse

The testcase is a bit misleading because this is the Construction Yard's make animation and not the MCV's, so it's reversed. But it's enough for testing

Screen.Recording.2022-12-09.at.14.48.36.mov

@abcdefg30
Copy link
Member

Confirmed that this still works. ✅

Btw, you can define

	make: factmake
		Length: *
		Frames: 31, 30, 29, 28, 27, 26, 25, 24, 23, 22, 21, 20, 19, 18, 17, 16, 15, 14, 13, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0

to fix the animation for the testcase.

Mailaender
Mailaender previously approved these changes Dec 31, 2022
@Mailaender
Copy link
Member

Mailaender commented Mar 11, 2023

Can you remove the test case? 62321f1

@PunkPun
Copy link
Member Author

PunkPun commented Mar 11, 2023

dropped the testcase

@Mailaender Mailaender merged commit 8a18c2e into OpenRA:bleed Mar 11, 2023
@Mailaender
Copy link
Member

Changelog

@PunkPun PunkPun deleted the deploy-check branch March 11, 2023 21:14
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.

MCV Deployment overlaps with adjacent MCVs
4 participants