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

GLES3: Mark CLIP_IGNORE command while batching #85533

Closed
wants to merge 1 commit into from

Conversation

HolySkyMin
Copy link
Contributor

@HolySkyMin HolySkyMin commented Nov 30, 2023

Fixes #74115

EDITED: This PR marks CLIP_IGNORE command while batching to prevent unintended drawing when a canvas item's drawing ends with this command.


First attempt(was not a good approach):

This PR allows TYPE_CLIP_IGNORE(only when it is saying false and actually changes clipping state) being into GLES3 canvas item batch array. In theory, TYPE_CLIP_IGNORE batch should do nothing, only updating clipping information inside _render_items().

The reason of not adding it when it is saying true is that it changes next batch's clipping data which updates clipping information inside _render_items(). So it is meaningless to add true state into batch array.

@HolySkyMin HolySkyMin requested a review from a team as a code owner November 30, 2023 10:44
@AThousandShips AThousandShips added this to the 4.3 milestone Nov 30, 2023
@AThousandShips AThousandShips added cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Nov 30, 2023
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

I'm not sure that this solution will always work. I think right now it appears to work because in the editor, the only place the CLIP_IGNORE is used has the following pattern:

RenderingServer::get_singleton()->canvas_item_add_clip_ignore(ci, true);
 // draw something
RenderingServer::get_singleton()->canvas_item_add_clip_ignore(ci, false);

So the clip ignore command is used in the batch with the drawing, and then the clip_ignore command is added at the end of the drawing and never gets properly unset (because a draw command isn't added after it).

The problem with adding a dummy instance like you do here is that if a draw call is added after this, then its going to draw an extra instance with bogus info.

@HolySkyMin
Copy link
Contributor Author

HolySkyMin commented Dec 1, 2023

Draw call is being added after it in ItemList and RichTextLabel(if I'm not misreading the code), and when I tested them I couldn't find any behaviors malfunctioning.

As I said in initial post, clip ignore command in render phase (after finished batching) should do nothing(only setting up the clip). 'Doing nothing' includes not polluting other draw calls after it. I think it is doing nothing currently, but if it is found out to do something, then we should find another way.

EDIT: I know this is hacky and less intelligent solution. I will find a better way - until then I want to leave this available as an option.

@HolySkyMin HolySkyMin changed the title GLES3: Add CLIP_IGNORE (false) command into render batch GLES3: Mark CLIP_IGNORE command while batching Dec 1, 2023
@HolySkyMin
Copy link
Contributor Author

HolySkyMin commented Dec 1, 2023

Found a better solution. Instead of adding batch, just marking CLIP_IGNORE command while batching will have the same result.

@clayjohn
Copy link
Member

clayjohn commented Dec 1, 2023

This will create a batch that does nothing but set the CLIP_IGNORE. However, the actually batching logic will still run. So there should be an early out from the batching loop that exists after updating the clip otherwise it will run the logic to bind the shader, set blend mode, bind texture, etc.

@HolySkyMin
Copy link
Contributor Author

Added early out while rendering batches. Now a batch with clip ignore command will be skipped immediately.

// Ignoring CLIP_IGNORE command.
// Normally this type of command should not exist in here, but
// in some cases it will exist as the debris of prior canvas item.
if (state.canvas_instance_batches[i].command_type == Item::Command::TYPE_CLIP_IGNORE) {
Copy link
Member

Choose a reason for hiding this comment

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

This should happen after clipping is applied, no? With this change, clipping would never get applied.

Copy link
Contributor Author

@HolySkyMin HolySkyMin Dec 2, 2023

Choose a reason for hiding this comment

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

CLIP_IGNORE command can't be recorded to the batch as far as I know(instance count remains zero and new batch will not be created in next command), except no other commands are following. So if it is alive in the batch, it's completely useless and can be trashed immediately I think.

@clayjohn
Copy link
Member

clayjohn commented Dec 7, 2023

Superseded by #85778

@clayjohn clayjohn closed this Dec 7, 2023
@clayjohn clayjohn removed this from the 4.3 milestone Dec 7, 2023
@clayjohn clayjohn removed cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Dec 7, 2023
@HolySkyMin HolySkyMin deleted the weird_box_killer branch December 8, 2023 17:01
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.

Draw issue with focus in UI elements (Compatibility renderer)
4 participants