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

Web: Disable raycast module by default (no occlusion culling) #81716

Merged
merged 1 commit into from
Sep 22, 2023

Conversation

akien-mga
Copy link
Member

@akien-mga akien-mga commented Sep 15, 2023

This means no CPU occlusion culling (and not compiling Embree), unless you compile custom export templates with module_raycast_enabled=yes.

This reduces the memory footprint significantly, and binary size.

Fixes #70621.

@Calinou I'm not too familiar with the occlusion culling and lightmap documentation, could you tell me where to add some notes that this is not supported in default Web templates?

@akien-mga akien-mga added bug platform:web topic:porting cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release labels Sep 15, 2023
@akien-mga akien-mga added this to the 4.2 milestone Sep 15, 2023
@akien-mga akien-mga requested a review from a team September 15, 2023 22:23
@akien-mga akien-mga requested a review from a team as a code owner September 15, 2023 22:23
@Calinou
Copy link
Member

Calinou commented Sep 15, 2023

and lightmap baking IIUC, unless you compile custom export templates with
module_raycast_enabled=yes.

The CPU lightmapper hasn't been reimplemented in 4.x yet, but we don't know if we'll keep Embree when it's ported anyway. (It also isn't available in exported projects, so it would only be usable in the web editor – and it would be slow due to WebAssembly overhead.)

I'm not too familiar with the occlusion culling and lightmap documentation, could you tell me where to add some notes that this is not supported in default Web templates?

Probably in these classes' descriptions:

And in these properties:

I can open a separate PR on godot-docs for https://docs.godotengine.org/en/latest/tutorials/3d/occlusion_culling.html.

Copy link
Contributor

@ryanabx ryanabx left a comment

Choose a reason for hiding this comment

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

I approve I approve I approve.

Most people exporting for web won't even know about the raycast module anyways, and this has been a compatibility issue with Webkit/iOS for a while now!

Pending the docs change, of course, but it looks like Calinou has already brought attention to that. It'd be good to put a notice in there for the person who comes by every so often who would need to know this.

@akien-mga akien-mga requested a review from a team as a code owner September 16, 2023 18:59
This means no CPU occlusion culling (and not compiling Embree), unless
you compile custom export templates with `module_raycast_enabled=yes`.

This reduces the memory footprint significantly, and binary size.

Fixes godotengine#70621.

Co-authored-by: Hugo Locurcio <hugo.locurcio@hugo.pro>
@akien-mga
Copy link
Member Author

Thanks @Calinou, added docs. I didn't put the note in Occluder3D and Viewport to avoid being too redundant, as both places refer to the other class or property for more details already.

@akien-mga akien-mga changed the title Web: Disable raycast module by default Web: Disable raycast module by default (no occlusion culling) Sep 16, 2023
@clayjohn
Copy link
Member

We use raycasts for importing meshes with LODs. We need to check that this change does not break mesh importing

@AThousandShips
Copy link
Member

How feasible is importing meshes with LOD on web editor?

@Calinou
Copy link
Member

Calinou commented Sep 17, 2023

How feasible is importing meshes with LOD on web editor?

It is technically feasible, but you probably don't want to work on a project with highly complex meshes in the web editor anyway.

@ryanabx
Copy link
Contributor

ryanabx commented Sep 17, 2023

How feasible is importing meshes with LOD on web editor?

It is technically feasible, but you probably don't want to work on a project with highly complex meshes in the web editor anyway.

In my opinion, the limitation should be clarified in documentation. I don't know if there is a section about using the web editor, but clarifying that would be helpful. Other than that, I do think this change is for the better.

@ryanabx
Copy link
Contributor

ryanabx commented Sep 20, 2023

I've tested importing a gltf scene into the godot web editor on this PR.

Sample model:
https://github.com/KhronosGroup/glTF-Sample-Models/raw/master/2.0/AnimatedCube/screenshot/screenshot.gif

It imported successfully: (ignore the watermark, had to compress the video)

cube-rotate_be9MIAJl.mp4

Let me know if any more testing is required!

@ryanabx
Copy link
Contributor

ryanabx commented Sep 20, 2023

Follow-up:
I was asked by @clayjohn to test LOD bias on an imported gltf model. I chose a dragon model from the same repo as the rotating cube.

LOD Bias = 1:
image

LOD Bias = 0.001 (Lowest):
image

The detail does in fact change on web with this PR! I say we move forward with this :)

EDIT: Better quality images

@clayjohn
Copy link
Member

Thanks @ryanabx for confirming that compiling out RayCast won't break our LOD import logic!

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.

Seems like a good change. If there is demand, we can add a slim StaticRayCast fallback implementation for memory constrained systems.

@akien-mga akien-mga merged commit bafcd32 into godotengine:master Sep 22, 2023
15 checks passed
@akien-mga akien-mga deleted the web-disable-raycast-embree branch September 22, 2023 20:06
@akien-mga
Copy link
Member Author

Cherry-picked for 4.1.2.

@akien-mga akien-mga removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Oct 3, 2023
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.

WebAssembly maximum memory 2GB causes Out of Memory error on iOS Safari 16.2
5 participants