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 overrideRenderingFillMode property to Mesh #13708

Merged
merged 2 commits into from
Apr 6, 2023

Conversation

sebavan
Copy link
Member

@sebavan sebavan commented Apr 5, 2023

Supersedes #13688

As discussed in forum thread here: https://forum.babylonjs.com/t/add-mesh-overridematerialfillmode/39475/

cc @MiikaH to review

The main difference is use the override only within the rendering scope as the flag is only used as a visual tool. It also fixes some of the scene override levels inconsistencies.

@bjsplat
Copy link
Collaborator

bjsplat commented Apr 5, 2023

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@bjsplat
Copy link
Collaborator

bjsplat commented Apr 5, 2023

@MiikaH
Copy link
Contributor

MiikaH commented Apr 6, 2023

Nice! That seems to work perfectly in my playground test case: https://playground.babylonjs.com/?snapshot=refs/pull/13708/merge#WX0F9G#2

That being said. In my forum post use case I do clone the mesh and change index buffer to special index buffer to render ngon wireframes. Material is shared, but geometry as a whole is not. However at least in my use case the override is only used for rendering so I have no complaints what-so-ever about this getting merged in a simplified render only form. :)

@sebavan
Copy link
Member Author

sebavan commented Apr 6, 2023

@Popov72 any thoughts ?

@Popov72
Copy link
Contributor

Popov72 commented Apr 6, 2023

I think we can merge this PR and see if there are other needs in the future?

@MiikaH
Copy link
Contributor

MiikaH commented Apr 6, 2023

Hmm.. I just noticed that you moved the fillMode assingment inside _processRendering().

That might be a problem? Right now:

  • Mesh.render() has unnecessary scene override assingment to the fillMode variable, only to be overridden again by the new call.
  • There are other areas of the code that call _processRendering() that may not want to apply the scene or mesh override? Such as VolumetricLightScatteringPostProcess

If this is kept as "render only" override, then it likely should be applied outside _processRendering() only for code paths that render final images (Mesh.render() and EffectLayer)?

@sebavan
Copy link
Member Author

sebavan commented Apr 6, 2023

There are other areas of the code that call _processRendering() that may not want to apply the scene or mesh override? Such as VolumetricLightScatteringPostProcess

It is more a bug in the previous implementation as it should use the requested fill mode and not force triangle which might even not fit with the content. (let s not that this might also be a problem with the override today but I prefer to wait for a REAL usage rather than introducing more code if it is never even used)

Mesh.render() has unnecessary scene override assingment to the fillMode variable, only to be overridden again by the new call.

Good catch, fixed in the latest push and checked in the code base.

@sebavan sebavan marked this pull request as ready for review April 6, 2023 16:43
@sebavan sebavan merged commit f8fe69b into BabylonJS:master Apr 6, 2023
@sebavan sebavan deleted the FillMode branch May 24, 2024 15:09
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.

4 participants