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 skybox to motion_blur example #13671

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

andristarr
Copy link
Contributor

Fixes #13632

@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Examples An addition or correction to our examples X-Contentious There are nontrivial implications that should be thought through labels Jun 4, 2024
@alice-i-cecile alice-i-cecile added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Jun 4, 2024
CREDITS.md Outdated Show resolved Hide resolved
CREDITS.md Outdated Show resolved Hide resolved
@alice-i-cecile
Copy link
Member

I like the choice of sky box, although AI-generation will be controversial even for test assets. CC-BY is a fine license.

@andristarr
Copy link
Contributor Author

I like the choice of sky box, although AI-generation will be controversial even for test assets. CC-BY is a fine license.

I can look for some other one if its prefered. Let me know what you think.

@alice-i-cecile
Copy link
Member

Everything else being equal, let's look for a different one. Criteria for me are a) human-made b) permissively licensed c) pretty d) mostly sky

@andristarr andristarr force-pushed the motion_blur.example.skybox branch 2 times, most recently from 762d7d9 to 95e193f Compare June 4, 2024 16:03
@aevyrie
Copy link
Member

aevyrie commented Jun 5, 2024

Ideally we would also have an envmap, so the lighting matches the skybox. You can use https://github.com/pcwalton/gltf-ibl-sampler-egui to generate skyboxes and specular/diffuse envmaps from hdr skyboxes you find online.

@andristarr
Copy link
Contributor Author

andristarr commented Jun 5, 2024

I found a CC0 skybox, but the suggested tool produces a rather large asset file. Let me know if you are content with this.

Copy link
Member

@aevyrie aevyrie left a comment

Choose a reason for hiding this comment

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

These should be compressed - see the readme on the pisa envmap in tree.
They should also be organized to match existing envmaps, there is already a directory that specifically stores envmaps.
Also, this should probably involve some of the rendering SMEs, quite a bit of discussion went into selecting the currently added envmaps, so it would be good to make sure it's on their radar.

Subjectively, this skybox is a bit ugly. We will be forced to keep assets small, so we may want to stick to skyboxes with low frequency detail only, e.g. no clouds, to avoid ugly pixelation.

@andristarr
Copy link
Contributor Author

@aevyrie can you tag someone from the said experts please? It would probably be beneficial if they give their input regarding picking a skybox then.

@aevyrie
Copy link
Member

aevyrie commented Jun 11, 2024

Hopefully I got this right: @superdump @pcwalton @JMS55 @IceSentry

@superdump
Copy link
Contributor

Could we use existing skybox and environment map textures in the repository to avoid having to add new ones? Having large binary files in the repository should have good motivation and if we already have something then maybe we don’t need to add more.

@alice-i-cecile
Copy link
Member

The motivation for this issue is that the existing assets are kind of ugly and don't work well for this particular case. @aevyrie can say more about what's lacking.

In general, I'd really like to move towards using nicer assets for our examples: they're the first thing people see and it's hard to disentangle "ugly/broken asset" from "ugly/broken engine".

@andristarr
Copy link
Contributor Author

andristarr commented Jun 16, 2024

To try to push this forward: with the currents assets (10 megs including all the files, including the suggested compression) it looks like the following:

2024-06-16.14-36-59.mp4

I will move them to the env maps folder so they sit together.

Let me know what you think

@aevyrie
Copy link
Member

aevyrie commented Jun 17, 2024

Unfortunately, 10 Mb is likely too large to be in tree, but that is not my call to make. Not sure what the limits or alternatives are.

As for the skybox itself, I would personally prefer we go with something lower detail, like other engines. I'd also expect that should be compressible down to a few Kb.

I have two issues with the skybox chosen here

  1. The fidelity isn't great, and doesn't leave a good impression with me. The highlights are blown out and hue shifted, and the clouds have a sort of dirty appearance. A photoreal skybox like this is likely always going to look terrible once it has been compressed down to an acceptable size to live in tree.
  2. The aesthetic doesn't seem to fit with the general feel of bevy. I'm not an authority here at all, this is my impression. Given the sort of "friendly" cartoony vibe of the logo and aesthetic elsewhere, I assume that when the org has the resources, it will be commissioning stylized assets, rather than photoreal ones? This is complete vibes-based speculation on my part though!

To give more a more concrete suggestion, if we are going to add a better default skybox, it should probably be something similar to other engines:

Fyrox
image
Unity
image
Godot

It's possible these are also just using a simple shader for the skybox and envmap, which would completely eliminate the need for a skybox texture.

@IceSentry
Copy link
Contributor

IceSentry commented Jun 17, 2024

I agree a default skybox like that would be better, but for the motion blur example, I'd would prefer a skybox with more stuff in it so you can actually see the blur. Otherwise we don't need to add one to that example.

@andristarr
Copy link
Contributor Author

andristarr commented Jun 18, 2024

I agree a default skybox like that would be better, but for the motion blur example, I'd would prefer a skybox with more stuff in it so you can actually see the blur. Otherwise we don't need to add one to that example.

I agree with the statement here. I was searching for free to use hdri skyboxes that would be potentially better in low res but I couldn't find one.

I am open for suggestions as I think it would look better with a nicer environment for this example.

@andristarr
Copy link
Contributor Author

I tried to create some skyboxes but none of them were able to jump the bare minimum of being acceptable so I am looking for input about this.

Copy link
Contributor

@pcwalton pcwalton left a comment

Choose a reason for hiding this comment

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

If you want to save space, try:

  • making the diffuse version smaller -- it's very blurry, you can safely shrink it down
  • using the specular part as the skybox. I know this is not correct, but it usually looks all right and it saves a lot of space. We do this in one of our examples.

@andristarr
Copy link
Contributor Author

With your suggestion I was able to remove the 4 megs of the cubemap.

I wouldnt scale down the resolution further because it gets really ugly :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Examples An addition or correction to our examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a simple skybox and envmap for use in examples
6 participants