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 ProxyDrawables not respecting whether the originals would be drawn #1507

Merged
merged 19 commits into from Apr 20, 2018

Conversation

3 participants
@smoogipoo
Contributor

smoogipoo commented Apr 10, 2018

Fixes #1283, fixes ppy/osu#1591.

Alternative to #1505.

Proxying the draw nodes seems to cover every case I can think of without requiring any parent traversal. Drawbacks to this:

  • Requires passing a frame counter down through GenerateDrawNodeSubtree/addFromComposite to determine if the drawnode was generated in the frame from the original drawable.
  • Adds around 24 bytes to ProxyDrawable.

So the way this all works now is ProxyDrawable now generates and adds a DrawNode to the subtree, but the only purpose of this specialised DrawNode is to proxy the DrawNode.Draw() call to the real DrawNode as generated by the original Drawable.

From:
screen shot 2018-04-10 at 6 53 32 pm

To:
screen shot 2018-04-10 at 6 52 31 pm

@smoogipoo smoogipoo referenced this pull request Apr 10, 2018

Merged

Remove now unnecessary approachcircle proxy disables #2374

1 of 1 task complete

@smoogipoo smoogipoo requested a review from Tom94 Apr 10, 2018

@smoogipoo

This comment has been minimized.

Contributor

smoogipoo commented Apr 16, 2018

I've now made ProxyDrawable a private class nested (in a partial class) in Drawable. It's exposed as a Drawable to consumers of CreateProxy now, with the now public members IsProxy/HasProxy if they need to specifically check for if it's aproxy

@Tom94

This comment has been minimized.

Collaborator

Tom94 commented Apr 16, 2018

I personally don't mind large files (your IDE should help deal with them easily) and thus would rather be in favor of not partialing Drawable at all. Alternatively, if it were to be partiald, then a lot more than just ProxyDrawable should be in a partial class.

@peppy

This comment has been minimized.

Member

peppy commented Apr 16, 2018

This particular case is where the partial part is only used to store a tightly coupled nested class. We do the same in osu! (BeatmapManager -> BeatmaapManagerWorkingBeatmap).

I'm fine with this usage of partial, but strongly against the same class being split across files. Hopefully @Tom94 can agree to this.

@Tom94

This comment has been minimized.

Collaborator

Tom94 commented Apr 16, 2018

I'm fine with that reasoning as long as we're consistent. How about the nested HandleInputCache?

@peppy

This comment has been minimized.

Member

peppy commented Apr 16, 2018

We generally only do it for larger classes, but even HandleInputCache is probably large enough to be moved out IMO. At the moment it's awkwardly wedged in the middle.

@smoogipoo

This comment has been minimized.

Contributor

smoogipoo commented Apr 16, 2018

+1 to moving HandleInputCache to a partial class too

@@ -676,73 +676,71 @@ protected override void ApplyDrawNode(DrawNode node)
base.ApplyDrawNode(node);
}
protected virtual bool CanBeFlattened => !Masking;
protected virtual bool CanBeFlattened => !Masking && !HasProxy;

This comment has been minimized.

@peppy

peppy Apr 18, 2018

Member

i'm not sure i follow why this was added here. may be worthwhile adding a local comment explaining it.

@peppy

This comment has been minimized.

Member

peppy commented Apr 18, 2018

Looks pretty good to me. @Tom94 can you please re-check this when you have some time?

@Tom94

This comment has been minimized.

Collaborator

Tom94 commented Apr 20, 2018

I am really uncomfortable with adding the complexity of the frame counter and validation along with DrawNode cross-references. I wonder if a nicer solution exists, because I'd like to keep the code of the Update/Draw pipeline as lean as possible.

On second thought I am also starting to doubt whether it makes sense to hide the Proxy when the parent is hidden, but not when it is masked away in the first place. That decision seems inconsistent. Since we also argued that partial masking would be impossible, I'd actually start leaning towards the old behaviour of not tying the Proxy to its Parent's state in general.

Lastly, we should also not ignore the possibility that ProxyDrawable may not be the optimal mechanism for the thing we're trying to achieve on the osu! side. Are there any alternatives? IIRC we didn't think very long and hard when we decided to add ProxyDrawable as a simple way to make approach circles work. If ProxyDrawable starts to get used in more complex ways and requires additional overhead we should re-evaluate if it's truly worth it.

@smoogipoo

This comment has been minimized.

Contributor

smoogipoo commented Apr 20, 2018

Re: Counter. I don't think the counter is too offensive, but we may be able to remove that in-exchange for a flag that gets set + reset on validation + on draw respectively. I'll experiment with this if you'd like.

Re complexity. I can't think of any other way to do this other than doing DrawNode cross references/validation, since you can't know whether the original Drawable would have been drawn until post-DrawNode-generation. Would be open to suggestion, however I've thought about this for many days and have attempted many things.

Re: Masked away. What do you mean by "not when it is masked away"? If the proxied drawable is outside of the proxy's parents' (which is masking) masking area, the proxied drawable will be "masked away" without IsMaskedAway being set to true.

E.g. with the following code:

new Container
{
    Children = new Drawable[]
    {
        box,
        new Container { Masking = true, Child = box.CreateProxy() }
    }
}

The following is possible (yellow is the masking container, red is box's proxy with the original box's position/size, white is the drawn part):
screen shot 2018-04-20 at 3 25 24 pm

Obviously you can imagine that as the box is moved more outside the container, less and less of it will be drawn, until a point where it is completely masked away. This stays true to the definition of IsMaskedAway which states "but it may be false, even if the Drawable was masked away" (i.e. in the above scenario, IsMaskedAway will be false).

You're free to suggest an alternative for osu!, however this doesn't exclude the fact that ProxyDrawables are inherently broken in their current state.

@Tom94

This comment has been minimized.

Collaborator

Tom94 commented Apr 20, 2018

  1. I don't have a better idea in mind; just wanted to make sure the design decision was thought about long and hard.

  2. I think you misunderstand. I'm talking about the case where the original is masked away by the original parent. I think it is inconsistent to keep drawing the proxy in this case, but to not keep drawing the proxy when the original parent hides the original in some other way than masking (e.g. by !IsPresent or !IsAlive). Either all of these cases should hide the proxy or none of them, and I favour "none", because "all" would require some sensible form of masking by the original, which we cannot do.

  3. I am not fully familiar with the context in which proxies are used within lazer and frankly I do not have the time to follow both lazer and framework development in detail. This makes it hard for me to come up with any specific ideas, so I leave the final decisions to you. Just giving my 2 cents on what I think may be the best course of action in case there is another way. IIRC for approach circles proxies were only used to customise draw order without throwing hitobject grouping under the bus; nothing else.

@smoogipoo

This comment has been minimized.

Contributor

smoogipoo commented Apr 20, 2018

  • When it is fully masked away by its parent, it's not going to be drawn by the proxy, since the original won't generate draw nodes. This is consistent with !IsPresent / !IsAlive.
  • When it is partially masked away by its parent (or even not masked away at all), the original would be drawn, so will be drawn by the proxy. The only difference is that the proxy is the one that has the final say as to how things appear on screen as it does the final drawnode masking.
@smoogipoo

This comment has been minimized.

Contributor

smoogipoo commented Apr 20, 2018

Your "none" proposal is potentially very dangerous, as that means you can remove a drawable from the hierarchy and still have it drawn by the proxy, meanwhile it's not getting any updates and may potentially even have no parent, leading to arbitrary DrawInfo states.

@Tom94

This comment has been minimized.

Collaborator

Tom94 commented Apr 20, 2018

Still sounds strange to me. If full masking by the original parent has any effect, why wouldn't partial masking have one, too?

Also, I find it to be a weird notion, that moving the original around without affecting how the proxy is ultimately drawn can result in the proxy suddenly becoming invisible. Seems like annoying mental overhead that is required when using proxies. As far as I can tell, as it stands now, proxied drawables are simply the originals inserted at other arbitrary positions within the scene graph without any strings attached. This sounds like the most straightforward model to me.

Could you refresh my mind on what the use case of the new behaviour would be within lazer?

@Tom94

This comment has been minimized.

Collaborator

Tom94 commented Apr 20, 2018

Re "none" proposal being dangerous: Should be trivial to throw an exception in such a case. We're already throwing exceptions when doing other operations on removed-from-scene-graph drawables.

@smoogipoo

This comment has been minimized.

Contributor

smoogipoo commented Apr 20, 2018

We can change the original to never have its IsMaskedAway updated, I wouldn't mind that on a first thought.

There are two immediate use cases for this in lazer:

  1. Approach Circles (as previously discussed), where you should be able to fastforward in the editor at any time and cause the original to become !IsAlive, and have it not be drawn.
  2. Swells (taiko), where they're proxied into an upper layer so they don't underlap the black underlay for the drum on the left. Upon fastforwarding in something like the editor, again, the hitobject will be in an !IsAlive state and should be hidden.

So your exception would be essentially "if the original is not drawn, throw an exception"? No way, that will cause so many issues and limit ProxyDrawable to being used only in completely static contexts.

@Tom94

This comment has been minimized.

Collaborator

Tom94 commented Apr 20, 2018

  1. Why not set the lifetime of the proxies to equal that of the originals? Should be a simple one-time thing.
  2. Same thing.

You were just talking about the case where drawables were removed from the scene graph. So was I. This has nothing to do with IsAlive unless the objects are actually meant to be destroyed upon becoming dead (i.e. RemoveWhenNotAlive == true), in which case an exception would be in order.

@smoogipoo

This comment has been minimized.

Contributor

smoogipoo commented Apr 20, 2018

Because lifetime is for the object itself, not for its parent. The ApproachCircle isn't the one that
becomes !IsAlive, it's the HitCircle that contains it.

Same for IsPresent.

smoogipoo and others added some commits Apr 20, 2018

@Tom94

Tom94 approved these changes Apr 20, 2018

As discussed in discord

@Tom94 Tom94 merged commit 72a55c7 into ppy:master Apr 20, 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