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 the ExplicitSequenceFilenames rule breaking when updating single maps #20857

Merged
merged 2 commits into from
May 30, 2023

Conversation

abcdefg30
Copy link
Member

Reported by @PunkPun on Discord. Testcase is adding the following to any D2k map.yaml and running the ExplicitSequenceFilenames rule on that map.

Sequences:
	frigate:
		icon: DATA.R8
			Start: 4290
			Offset: -30,-24

Bleed will give:

Sequences:
	frigate:
		icon:
			Filename: DATA.R8
			Start: 4290
			Offset: -30,-24
		idle:
			Filename: frigate

And running it a second time will produce:

Sequences:
	frigate:
		icon:
			Filename: frigate
			Filename: DATA.R8
			Start: 4290
			Offset: -30,-24
		idle:
			Filename: frigate
			Filename: frigate

The expected result in both cases is:

Sequences:
	frigate:
		icon:
			Filename: DATA.R8
			Start: 4290
			Offset: -30,-24

Seems like #20580 both introduced Filename and the update rule, so it is save to say that sequence nodes that already have a Filename node are already processed and can be ignored.

The issue in the example happens because the mod sequence idle is already updated and the map only defined the extra icon sequence. The update rule was trying to update the already updated sequence from the mod code again instead of just the map sequence.

@abcdefg30 abcdefg30 added this to the Next Release milestone May 11, 2023
@@ -330,6 +336,10 @@ public override IEnumerable<string> UpdateSequenceNode(ModData modData, MiniYaml

void ProcessNode(ModData modData, MiniYamlNode sequenceNode, MiniYamlNode resolvedSequenceNode, string imageName)
{
// "Filename" was introduced with this update rule, so that means this node was already processed and can be skipped
if (sequenceNode.LastChildMatching("Filename") != null)
return;
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure this won't break the rule for updating mods? This doesn't queue changes like #20861 does

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we are processing the same node several times? The update rule doesn't behave properly when this node already exist anyway.

Copy link
Member

Choose a reason for hiding this comment

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

I meant in regards to reading defaults node. And it also seems this is completely unnecessary change. Update rules are not meant to be run on the same yaml twice. It's a waste of effort to try to make that a possibility.

Copy link
Member Author

Choose a reason for hiding this comment

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

They are not meant to but might be. That is how this bug was detected after all. This PR would have been avoided if not for this very dismissive attitude.

Copy link
Member

@PunkPun PunkPun May 29, 2023

Choose a reason for hiding this comment

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

In quite some cases, it's impossible or extremely hard to make rules rerun-proof. And since we won't make sure all are safe, I don't think it makes any sense to put in effort into making any of them rerun-safe.

And adding to that reruns are a user mistake, one that is very easily avoided.

I do want to make rules to require as little manual intervention as possible. Fleshing them out as much as possible, but accounting for user mistakes? I think is a bit much.

Copy link
Member Author

Choose a reason for hiding this comment

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

The change has already been made. You are arguing to put more effort into this. I'm not saying we should go out of our way to build super complicated update rules, but the change is very easy here and good practice.

Copy link
Member

Choose a reason for hiding this comment

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

There is still effort left to put. We need to test whether this doesn't break mod updating

Copy link
Member Author

Choose a reason for hiding this comment

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

a) Impossible, see the comment.
b) I already did regardless, and the automatic rules produce the same result as #20580.

Copy link
Member

Choose a reason for hiding this comment

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

ok I'm going to trust that

@PunkPun PunkPun merged commit 95f18d4 into OpenRA:bleed May 30, 2023
3 checks passed
@abcdefg30 abcdefg30 deleted the fixSequenceUpdate branch May 30, 2023 13:32
@PunkPun
Copy link
Member

PunkPun commented May 30, 2023

Changelog

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

3 participants