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

Adding clear_method as a RendererOptions #606

Closed
wants to merge 1 commit into from

Conversation

@jaredpetker
Copy link
Contributor

jaredpetker commented Nov 29, 2016

This adds ClearMethod flags which can be used to select what type of method for clearing would like to be used during rendering, and adds an option to RendererOptions.

I went with flags vs just a boolean for clearing (yes / no) as I found there seemed to be different clears going on throughout the rendering (CLEAR_ALL, CLEAR_BACKGROUND, CLEAR_TILES, CLEAR_NONE). However, I don't currently have a great reason on why you would choose to only clear the background and not do tile clearing. My need was for the CLEAR_NONE option, however I thought this might be more useful than just a boolean. Happy to reconsider this via someone that is more familiar with the current direction of the project.

Issue Reference

@nical @kvark @glennw (tagging those that posted on the issue this far)


This change is Reviewable

- Gives the ability to select the method used for clearing
  during various portions of the rendering
@jaredpetker jaredpetker changed the title Adding clear_method as a RendererOption Adding clear_method as a RendererOptions Nov 29, 2016
@nical
Copy link
Collaborator

nical commented Nov 30, 2016

See also PR #605. I don't mind going for this one over PR #605 but I would prefer that the decision to add or not an opaque rectangle behind each pipeline be taken by passing the pipeline's background color as an option on a per-pipeline basisis rather than the flags in the renderer options, otherwise it means that depending on the flags you pass, a parameter of set_root_displaylist may or may not be ignored which is error prone. The flag in the renderer options should still control whether the renderer calls glClear on the main framebuffer

@jaredpetker
Copy link
Contributor Author

jaredpetker commented Nov 30, 2016

Ah, yea, I see the overlap between the two PR's. I can look into taking your comment / PR into account and making the alterations here.

@nical
Copy link
Collaborator

nical commented Nov 30, 2016

@jaredpetker or you can rebase on top of PR #605 , (although I think it already has everything you need). It incorporates controlling the backrounds at the pipeline level. It also lets you disable the clear shader which makes sense if the clear color is not opaque white (for example to draw on a semitransparent target) or when running on a tiled-deferred gpu where we are better off using glClear.

@jaredpetker
Copy link
Contributor Author

jaredpetker commented Nov 30, 2016

Can do makes sense. @nical is your thought basically then taking what you have but keeping the bitflags approach over the booleans used in that pr?

To be transparent (no pun intended) on my use case, I'm wanting to use webrender to essentially draw over existing graphics, which is why I'm looking for options to take away all clearing that is done.

@nical
Copy link
Collaborator

nical commented Nov 30, 2016

As far as I'm concerned, feel free to replace the booleans for bitflags in the RendererOptions I don't mind either way (the reviewer here is @glennw).
On top of PR #605, for your use case, just set clear_empty_tiles and clear_framebuffer to false (or the equivalent bitflags), and pass None for the background_color of the pipelines that need to have transparent backgrounds.

@jaredpetker
Copy link
Contributor Author

jaredpetker commented Nov 30, 2016

Ah interesting got it, let me just try out the other PR myself then and confirm it works for my use case, if that's the case then perhaps there's no need for mine here.

@bors-servo
Copy link
Contributor

bors-servo commented Dec 1, 2016

The latest upstream changes (presumably #612) made this pull request unmergeable. Please resolve the merge conflicts.

@glennw
Copy link
Member

glennw commented Dec 12, 2016

@jaredpetker OK, I've r+'ed the other similar PR to this. Once you confirm it contains the functionality you need and is working, I guess we can close this one? Thanks for working with @nical to get this functionality added! :)

@glennw
Copy link
Member

glennw commented Dec 14, 2016

Closing since similar functionality has merged in another PR. Please re-open if this is still required.

@glennw glennw closed this Dec 14, 2016
@jaredpetker
Copy link
Contributor Author

jaredpetker commented Dec 14, 2016

Missed the comments, have just been using my fork for my use case, haven't tried out the (what is now) merged, but I think it will do what I need / can add what is needed if that's not the case, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.