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

onBeforeRender called twice per render when passing a Mesh to the renderer. #19477

Closed
2 of 11 tasks
sjpt opened this issue May 27, 2020 · 8 comments · Fixed by #19487
Closed
2 of 11 tasks

onBeforeRender called twice per render when passing a Mesh to the renderer. #19477

sjpt opened this issue May 27, 2020 · 8 comments · Fixed by #19487

Comments

@sjpt
Copy link

sjpt commented May 27, 2020

Description of the problem

When onBeforeRender is applied to a mesh, and that mesh is rendered directly (not via a scene), then the onBeforeRender callback is applied twice for each render call.

I appreciate that it is not standard practice to render a mesh directly, but there are cases where it is helpful to avoid (the very small) overheads in performance, and in code noise.

https://jsfiddle.net/sjpt/t664p2yn/

Three.js version
  • [x ] Dev
  • [x ] r116
  • ...
Browser
  • All of them
  • Chrome
  • Firefox
  • Internet Explorer
OS
  • All of them
  • Windows
  • macOS
  • Linux
  • Android
  • iOS
Hardware Requirements (graphics card, VR Device, ...)
@Mugen87
Copy link
Collaborator

Mugen87 commented May 27, 2020

I think we have to change this line:

scene.onBeforeRender( _this, scene, camera, renderTarget || _currentRenderTarget );

to

if( scene.isScene ) scene.onBeforeRender( _this, scene, camera, renderTarget || _currentRenderTarget ); 

and do the same thing for onAfterRender().

@WestLangley
Copy link
Collaborator

I think we have to change this line:

The renderer expects a Scene as the first argument, so we do not have to do anything.

If you do not think a Scene as the first argument is appropriate, then the first step is to change the API.

@Mugen87
Copy link
Collaborator

Mugen87 commented May 27, 2020

@mrdoob recently implemented a342f06 or 4cc880d so I think applying 3D objects directly to render() is valid.

@mrdoob mrdoob added this to the r118 milestone May 27, 2020
@mrdoob mrdoob changed the title onBeforeRender called twice per render onBeforeRender called twice per render when passing a Mesh to the renderer. May 27, 2020
@sjpt
Copy link
Author

sjpt commented May 28, 2020

Associated comment. onBeforeRender() does not seem to get called when applied THREE.Group.

@Mugen87
Copy link
Collaborator

Mugen87 commented May 28, 2020

That's correct. It's only called for renderable 3D objects and scenes.

@sjpt
Copy link
Author

sjpt commented May 28, 2020

Thanks. That should be clearer in the documentation.

Is there any reason not to apply it to groups?
I have group Group consisting of objects Wire and Shaded.
Both Wire and Shaded require the same preparatory work Prep().
I had assumed that setting Group.onBeforeRender=Prep would do that.

@Mugen87
Copy link
Collaborator

Mugen87 commented May 28, 2020

Is there any reason not to apply it to groups?

This was already discussed here: #11306

@sjpt
Copy link
Author

sjpt commented May 28, 2020

That #11306 thread seems to say that it should work,
'mrdoob added this to the rXX milestone on 9 Mar 2018'

Came up again in #14970 which was closed in favour of #11306, but #11306 still isn't resolved.

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 a pull request may close this issue.

4 participants