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

Add Background.from_color #733

Merged
merged 4 commits into from
Apr 16, 2024
Merged

Add Background.from_color #733

merged 4 commits into from
Apr 16, 2024

Conversation

almarklein
Copy link
Collaborator

@almarklein almarklein commented Apr 16, 2024

Intro

To avoid blending issues (as in #724) it should be recommended to use a background in most use-cases. But creating a simple uniform background is currently a bit verbose:

scene.add(gfx.Background(None, gfx.BackgroundMaterial("#000")))

This is because the Background has the same geometry+material args as all world objects (except Scene and Group). I value that consistency, but it's also annoying to write out.

Alternative options

One option could be to drop the Geomety arg. But that's still pretty long. And it creates a problem when at some point we get a new kind of background that does need geometry.

scene.add(gfx.Background(gfx.BackgroundMaterial("#000")))

Another option is to allow something like this. I like the brevity, but documenting this is a bit awkward, I don't think this level of dynamic arguments works well here. Also what about a gradient?

scene.add(gfx.Background("#000"))

So what I came up with is this. A bit longer than the previous, but in return its explicit and unambiguous.

scene.add(gfx.Background.from_color("#000"))

Tasks

  • Agreement on this proposal.
  • Update examples that currenly use a color background.

Korijn
Korijn previously approved these changes Apr 16, 2024
@panxinmiao
Copy link
Contributor

How about adding a background property to the scene?

Its value can accept Color, Image, Texture(CubeTexture) and Background, and pygfx will automatically handle the conversion.

With this, we can use the syntax scene.background = (0, 0, 0)

@almarklein
Copy link
Collaborator Author

@panxinmiao That is an interesting idea, which is also discussed in #582 (and other issues linked therein). A decision about that has not been made yet. We want to iron out other parts of the API first. But for the time being we prefer to keep the Scene no more than a group object.

The current PR also does not exclude that path, if we want to go that direction later.

@almarklein almarklein marked this pull request as ready for review April 16, 2024 10:38
@almarklein
Copy link
Collaborator Author

To add to that, I really like scene.background = 'red', but if we go that path, we should consider other properties (lights, clipping planes, etc.) too, and that's a pretty profound API change.

Copy link
Collaborator

@Korijn Korijn 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 really a fan of scene.background... I prefer API consistency

@Korijn Korijn merged commit 7ded6bc into main Apr 16, 2024
12 checks passed
@Korijn Korijn deleted the Background-from_color branch April 16, 2024 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants