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

Fixing layering order #6882

Merged
merged 1 commit into from
May 30, 2023
Merged

Fixing layering order #6882

merged 1 commit into from
May 30, 2023

Conversation

wdanilo
Copy link
Member

@wdanilo wdanilo commented May 30, 2023

Pull Request Description

This might fix #6874 . I can't reproduce the issue, but I understand why it might happen, as the ordering was not defined explicitly.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

@wdanilo wdanilo added the CI: No changelog needed Do not require a changelog entry for this PR. label May 30, 2023
@kazcw
Copy link
Contributor

kazcw commented May 30, 2023

I wonder if we have other undeclared dependencies that could break at any time. It's hard to be sure every needed dependency is declared, but that wouldn't be as much of a problem if ties in the layer item toposort were always resolved the same way. The DependencyGraph toposort looks deterministic per se, but the items its sorting are not: SymbolIds are sequentially-assigned (timing-dependent); ShapeSystemIds are TypeIds (ordering is completely up to the compiler). If we could either make these both have a deterministic total order, or assign stable keys for the toposort, we could ensure that the order we're testing is the same order as everyone will see. For the ShapeSystemIds, we could use their definition_path as a stable ID.

Copy link
Contributor

@farmaazon farmaazon left a comment

Choose a reason for hiding this comment

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

Another approval for your impressive collection

@farmaazon farmaazon added the CI: Ready to merge This PR is eligible for automatic merge label May 30, 2023
@mergify mergify bot merged commit 53887b2 into develop May 30, 2023
@mergify mergify bot deleted the wip/wd/layer_order_fix branch May 30, 2023 10:21
@wdanilo
Copy link
Member Author

wdanilo commented May 30, 2023

@kazcw I was thinking exactly about that yesterday and I think we should emit warnings in case we try to render things on the same layer without explicit ordering. This would catch such bugs way earlier and we already had a few regressions exactly because of this issue.

Of course, if we could assign automatically some stable IDs, that would be an amazing idea, however, they could not change when refactoring a code. For example, after moving files around, the ordering of the layers should not change, which makes it a little bit harder. To you see any way of doing it, or do you feel that requiring explicit ordering is the best solution here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Warning and Error layer ordering on nodes went wrong
4 participants