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

Make yaml node resolving optional #20860

Merged
merged 1 commit into from Jul 30, 2023
Merged

Conversation

PunkPun
Copy link
Member

@PunkPun PunkPun commented May 15, 2023

Partially fixes #20847

As we can't come up with a perfect solution the goal of this PR is just to make the situation much better.

  • The first solution is to resolve rules as infrequently as possible with the introduction of IBeforeUpdate* interfaces.
  • Second solution is to delay revolving in hopes that other update rules will fix the causes for crash

As a side effect this should make the update rules run faster as we resolve way way less yaml

penev92
penev92 previously approved these changes Jul 18, 2023
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.

The IBeforeUpdate* interfaces introduction feels like a no-op honestly. I don't mind it, I just feel it doesn't help anything.
The fix to the crash seems to be the reordering of the update rules, which as @abcdefg30 said is pretty ... magical.
That said, I think just reordering is fine and we should just be mindful of this with future update rules because anything else would likely be overkill and I have no better suggestion anyway.

@PunkPun
Copy link
Member Author

PunkPun commented Jul 18, 2023

Introduction of IBeforeUpdate* is as much of a fix as reordering. Updated the PR's description to an include an explanation

@PunkPun
Copy link
Member Author

PunkPun commented Jul 25, 2023

rebased

@Mailaender
Copy link
Member

Needs a rebase.

@PunkPun
Copy link
Member Author

PunkPun commented Jul 26, 2023

rebased

@Mailaender Mailaender merged commit 462a3ef into OpenRA:bleed Jul 30, 2023
3 checks passed
@Mailaender
Copy link
Member

Changelog

@PunkPun PunkPun deleted the fix-rules branch July 30, 2023 19:00
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.

Utility should not crash before update rules are ran
6 participants