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

Fix for RA2 Mod: cloning vats #17434

Merged
merged 1 commit into from Dec 12, 2019
Merged

Conversation

@torleif
Copy link
Contributor

torleif commented Dec 8, 2019

Fixes #17396

This fix will allow cloning vats in the RA2 mod to work correctly.

The RA2 mod currently has a bug where the cloning vats don't work. This is because they're set to have a production type of Dummy, so that they can produce units. However there is a check in the OpenRA engine prevents new units of only the same type being produced. Because cloning vats should not be able to allow the construction of new units (you need a barracks to do this), it's set to a fake Dummy type.

I was tempted to rip out the check altogether, however I'm unsure if other mods have buildings that use the cloning feature.

I've pulled the check out of the main function into its own function for readability.

@torleif torleif changed the title For for RA2 Mod: cloning vats Fix for RA2 Mod: cloning vats Dec 8, 2019
@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Dec 8, 2019

This is entirely wrong on principle.

The actual issue why the Vats break is #17396 (which this doesn't fix currently) and should be considered as a few-line fix - you need to add a ProductionType string with FieldLoader.Require property and then change the last Produce call within the UnitProducedByOtherfunction to use this new ProductionType instead of the original.

See https://github.com/AttacqueSuperior/Engine/blob/master/OpenRA.Mods.AS/Duplicates/Traits/Buildings/ClonesProducedUnitsAS.cs as reference.

You can also look at https://github.com/AttacqueSuperior/ValiantHearts/blob/master/mods/ra2vh/rules/soviet-structures.yaml#L578 to see how should the YAML look like when this is fixed in the proper method.

@Mailaender

This comment has been minimized.

Copy link
Member

Mailaender commented Dec 8, 2019

The code should probably move to https://github.com/openRA/ra2.

@torleif

This comment has been minimized.

Copy link
Contributor Author

torleif commented Dec 9, 2019

Thanks for your feedback. @GraionDilach I've rewritten the patch so it uses ProductionType instead.

I've tested it with the following yml and it works as expected:

	Production:
		Produces: Clone
	ClonesProducedUnits:
		CloneableTypes: infantry
		ProductionType: Clone
Copy link
Member

abcdefg30 left a comment

Although untested, the code changes look good to me. Can you squash the commits, please?

formatting

minor formatting

passing CI

closing line bracket

use production unit

opening should not be followed by a blank line

revert tab change
@torleif torleif force-pushed the torleif:torleif/cloning-vat-fix branch from 980e741 to 7d5f937 Dec 10, 2019
@torleif

This comment has been minimized.

Copy link
Contributor Author

torleif commented Dec 10, 2019

squashed commits

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Dec 10, 2019

Please edit the initial post to include the text "Fixes #17396" for ticket autoclosure.

@torleif torleif changed the title Fix for RA2 Mod: cloning vats Fix for RA2 Mod: cloning vats Fixes #17396 Dec 10, 2019
@torleif torleif changed the title Fix for RA2 Mod: cloning vats Fixes #17396 Fix for RA2 Mod: cloning vats Dec 10, 2019
@reaperrr reaperrr merged commit ae4b259 into OpenRA:bleed Dec 12, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Dec 12, 2019

@torleif torleif deleted the torleif:torleif/cloning-vat-fix branch Dec 16, 2019
@Mailaender Mailaender mentioned this pull request Dec 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

7 participants
You can’t perform that action at this time.