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

Rewrite yaml merger #10648

Merged
merged 4 commits into from
Jan 30, 2016
Merged

Rewrite yaml merger #10648

merged 4 commits into from
Jan 30, 2016

Conversation

pchote
Copy link
Member

@pchote pchote commented Jan 30, 2016

Fixes #10207
Fixes #7556
Fixes #8890
Fixes #8086
Fixes #8185

As I explained in #10207 all these bugs exist because the code tried to treat yaml merging and inheritance as separable steps (which is completely bogus). This moves the inheritance logic into the yaml parser and fixes the merge and removal logic to behave in the logical way.

var a = MiniYaml.FromString(mixedMergeA, "mixedMergeA");
var b = MiniYaml.FromString(mixedMergeB, "mixedMergeB");

// Merge order should not matter
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 removed this test because it was bogus (merge order does matter).

@pchote pchote force-pushed the rewrite-yaml-merger branch 2 times, most recently from fd62380 to acf7737 Compare January 30, 2016 14:37
@pchote
Copy link
Member Author

pchote commented Jan 30, 2016

I don't understand the thf and hijacker lint errors on bomber john and drop zone w. Neither of those maps touch anything related to infantry.

Edit: Both of those cases use -IdleSequences: to clear the idle animations. Some factor must be breaking the yaml merge only on those two maps

@pchote
Copy link
Member Author

pchote commented Jan 30, 2016

Found the issue, wrote a unit test for it, then fixed it.

@reaperrr
Copy link
Contributor

#8086 seems to be fixed.

#8185 is not fixed.
For this change in RA's voice.yaml

    ^DefaultDeathVoices:
    Voices:
        Die: dedman1,dedman2,dedman3,dedman4,dedman5,dedman7,dedman8
        Burned: dedman10
        Zapped: dedman6
    DisableVariants: Die, Burned, Zapped

    GenericVoice:
    Inherits: ^DefaultDeathVoices
    Variants:
        allies: .v01,.v03
        england: .v01,.v03
        france: .v01,.v03
        germany: .v01,.v03
        soviet: .r01,.r03
        russia: .r01,.r03
        ukraine: .r01,.r03
    Voices:
        Select: await1,ready,report1,yessir1
        Action: ackno,affirm1,noprob,overout,ritaway,roger,ugotit

I got

Exception of type System.InvalidOperationException: Can't find Die in voice pool.

edit: Ignore the partially wrong indentation, the .yaml has correct indentation.

@pchote
Copy link
Member Author

pchote commented Jan 30, 2016

@reaperrr: did you try that before or after my latest fixup? The issue with -IdleSequences that I have just fixed would have also caused what you just described.

Edit: Just tested myself, and confirmed that voice inheritance does indeed work. Updated the fixes list at the top to reflect this.

@reaperrr
Copy link
Contributor

Before, works now.

@reaperrr
Copy link
Contributor

Tested weapon and sequence inheritance, encountered no issues.

The code is out of my league, but the way it works in practice gets my 👍

@obrakmann
Copy link
Contributor

Looks fine to me. The only inconsistency I could find was this:

rules/aircraft.yaml:

U2:
        -Selectable:

in map.yaml:

U2:
        -Selectable:

doesn't throw an error for the double removal.

U2.copy:
      Inherits@u2: U2
      -Selectable:

does throw an error.

I guess that's because of this call to Union, which would merge all duplicate lines for an actor entry. I'm fine with that. Leaving the comment here for a bit to see if anyone else complains, otherwise this gets my 👍 as well.

@pchote
Copy link
Member Author

pchote commented Jan 30, 2016

Correct. Merging all keys with the same identifier together is fundamental to the way our rules are set up. I don't think there's any reason to handle removals differently.

obrakmann added a commit that referenced this pull request Jan 30, 2016
@obrakmann obrakmann merged commit 7ca1af4 into OpenRA:bleed Jan 30, 2016
@obrakmann
Copy link
Contributor

Changelog

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants