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 children life update in BDL-load #1542

Merged
merged 6 commits into from May 11, 2018

Conversation

2 participants
@smoogipoo
Contributor

smoogipoo commented May 8, 2018

The issue that sparked this: ppy/osu#2496

Explanation:
DrawableMedal binds to medalContainer.OnLoadComplete, to set the position of some text contained within that container (i.e. unlocked.Position = new Vector2(0f, medalContainer.DrawSize.Y / 2 + 10);).

This is expecting two things:

  1. The children of medalContainer are loaded.
  2. The children of medalContainer is alive.

(1) Is fulfilled by LoadComplete being called from UpdateSubTree, which occurs post-all-children-being-loaded.
(2) Is where the issue occurs. CompositeDrawable.load() called UpdateChildrenLife, but it would get blocked by load state check in checkChildLife: https://github.com/ppy/osu-framework/compare/master...smoogipoo:async-child-life?expand=1#diff-2f079688019880634dc5249946a02502L407.
The next time child life gets updated is post-LoadComplete. So the children of medalContainer were loaded, but not alive.
This results in autosize not finding alive children and giving a DrawSize of 0.

A similar change as proposed in this PR was submitted some ~6 months ago (#1072) but then reverted (#1083). The reasoning was that some elements were broken (https://streamable.com/6pwqf, ppy/osu#1366). Although these elements are not broken with the same changes as in the original PR, I chose to go down a different route and maintain that "children are not alive or loaded before their parent is loaded".

There are a few significant changes here:

  1. checkChildLife no longer does load state checking. This is now done in UpdateChildrenLife and AddInternal - the only other use-case of checkChildLife through publicly-acccessible methods.
  2. The previous functionality of UpdateChildrenLife has been moved to a new private method checkChildrenLife. The new method, like checkChildLife, does not perform load state checking.
  3. BDL-load now uses checkChildLife, bypassing the load state checking of UpdateChildrenLife.

Additionally, I think the following will result in some discussion:
I've moved the call to check children life to LoadAsyncComplete, which happens after BDL-load,
but still on the asynchronous context, and prior to the drawable being given a Ready load state.
This was done because the alive state of children defined in BDL-load would previously be computed on the Update thread, when UpdateChildrenLife would be normally invoked.

Testing

It's hard to define exactly what to test with this. Obviously I've tested that it fixes the issue that sparked this, but I've also gone through all visual tests (osu! and framework-side) and found nothing breaking.

smoogipoo added some commits May 8, 2018

Move child life checking to LoadAsyncComplete
This changes two things:
1) We generally derive CompositeDrawable and add hierarchies in load(), this will cause those children to now potentially turn alive if they need to be. Previously only children added during non-bdl-load would turn alive during bdl-load.
2) Similar to (1), drawables added during derived bdl-loads that should be alive will now get loaded.

@smoogipoo smoogipoo added this to the May 2018 milestone May 8, 2018

@peppy peppy requested review from peppy and Tom94 May 8, 2018

@smoogipoo

This comment has been minimized.

Contributor

smoogipoo commented May 8, 2018

osu load sequence-5

Made a flow chart of the loading procedure after this change, probably to be used in the wiki.

@smoogipoo

This comment has been minimized.

Contributor

smoogipoo commented May 8, 2018

osu load sequence-4-2

Updated flow chart.

Raw for future reference:
osu! load sequence-4-3.xml.zip

@smoogipoo

This comment has been minimized.

Contributor

smoogipoo commented May 8, 2018

@peppy

This comment has been minimized.

Member

peppy commented May 10, 2018

@peppy

peppy approved these changes May 10, 2018

@peppy peppy merged commit b3bacde into ppy:master May 11, 2018

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment