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 basic cubemap support. #240

Closed
wants to merge 7 commits into from
Closed

Conversation

charles-l
Copy link

@charles-l charles-l commented Aug 9, 2020

While working on a game prototype using kiss3d, I realized one thing that was missing in the base library was cubemap/skybox support. I think adding support for this is generally useful for 3D prototyping, and if it's possible with a one liner, all the better.

Still TODO:

  • WASM support
  • Make typesafe
  • Internalize skybox material
  • Benchmark startup time (not sure where this is coming from)
  • Add skybox texture to demo (cat pictures are cute, but it really ought to demo how different sides can be shown)
  • Docs

@charles-l
Copy link
Author

Hmm... WASM support might be tricky -- I'm having difficulty getting the example WASM project to compile...

@charles-l
Copy link
Author

Ran a quick profiling test on the cube_map.rs example to try and figure out what the startup time was coming from. Apparently, the PNG decoder is pretty slow (according to my profiling results 96% of the startup time is spent in the png decode function). Probably not worth digging into at the moment. I just noticed that a lot of time was being spent in the InflateStream code, so this issue might be related image-rs/image-png#114

Anyway, it goes beyond the scope of this PR and the only short-term modification I can think of making is multithreading the cubemap loading function which is probably more trouble than it's worth.

@charles-l
Copy link
Author

@sebcrozet some quick questions for you when you get a chance:

  1. Is WASM support ready? I couldn't get the example project to build, and I saw some recent activity around WASM support so I'm not sure if it's still in progress. But if it's ready, I'll dig into my issue further and make sure this PR supports WASM. Otherwise, I think it makes sense to make web cubemap support a secondary PR later once WASM stuff is fully fleshed out.
  2. Currently, the Cubemap texture is stored in the CubemapMaterial structure rather than the ObjectData.texture field (https://github.com/sebcrozet/kiss3d/pull/240/files#diff-d6cc39930e971e874f1b95f5064596cbR31). This is certainly at odds with the rest of the API design, but I'm not sure of a better way of implementing this at the moment. I wanted to avoid slapping a new member on the ObjectData field, or making the ObjectData.texture field a variant type. Besides, cubemaps are generally bound to cube meshes, so I'm not sure putting cubemap info in the generic ObjectData struct which can be associated with other meshes quite makes sense either. Maybe it would be worth creating an entirely new type with a hardcoded cube mesh and cubemap texture?
  3. The build is failing on the rustfmt step. I tried running rustfmt across the project, but that changed almost every file, and I didn't want to submit a noisy diff. Before merging, I can run it across the new files I've added, but I'm not sure that'll fix the build.

Sorry for the big PR -- this ended up unfurling into more changes than I was expecting. When I convert it from a draft PR, I can create a few stacked PRs for more sane chunks of work (i.e. make bind_texture only use TEXTURE_2D, add cubemap support to the context, add the Cubemap material, etc).

@alvinhochun
Copy link
Collaborator

alvinhochun commented Aug 22, 2020

  1. Is WASM support ready? I couldn't get the example project to build, and I saw some recent activity around WASM support so I'm not sure if it's still in progress. But if it's ready, I'll dig into my issue further and make sure this PR supports WASM. Otherwise, I think it makes sense to make web cubemap support a secondary PR later once WASM stuff is fully fleshed out.

WASM support through stdweb should work (it had been there for quite some time). I believe @sebcrozet mainly used cargo-web though I've had success with wasm-pack (which requires a patch to stdweb.

The recent work #241 is about moving away from stdweb to web-sys which means ditching cargo-web and only supporting wasm-pack (or manual wasm-bindgen). But you probably want to base your work on that branch as @sebcrozet had indicated the intention to merge it soon.

@RaduBerinde
Copy link
Contributor

Some comments:

  1. This makes some backward incompatible API changes which might break existing users (e.g. bind_texture). The changes seem mostly cosmetic so it would be preferable to avoid them.
  2. Given that we're using the OpenGL support for cubemapping, we don't need to actually render the cube itself. We can simply render a full-screen planar quad at z=0.99. This has the advantage that the user doesn't need to set a cube size, and more importantly we don't waste 30% of the depth range - the area where objects might clip the cube.

I realize this PR is very old, but I would be willing to pick it up and make these modifications if there is interest (i.e. someone wants to review).

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