edited

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.

This comment has been minimized.

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

This comment has been minimized.

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.

This comment has been minimized.

edited

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.

This comment has been minimized.

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.

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):

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.

This comment has been minimized.

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

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.

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.

This comment has been minimized.

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.

This comment has been minimized.

edited

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.

This comment has been minimized.

edited

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?

This comment has been minimized.

edited

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:

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.

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.

This comment has been minimized.

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

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.

1 check passed

1 check passed

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.