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

WebGLRenderer: Add #pragma unroll_loop_start/end syntax. #18804

Merged
merged 5 commits into from Mar 6, 2020

Conversation

gkjohnson
Copy link
Collaborator

As mentioned in #18751 (comment), this PR replaces #pragma unroll_loop with #unroll_loop_start and #unroll_loop_end to denote the beginning and end of the loop.

This logs a warning when #pragma unroll_loop is used and updates both the CSM shader and light_fragment_begin shader to demonstrate that it works.

In another PR I can upgrade the rest of the shaders and the docs if this is accepted.

@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 4, 2020

Just for the record: It's necessary to update the docs of ShaderMaterial. There is a section that explains the usage of #pragma unroll_loop.

Besides, TranslucentShader has to be migrated:

" #pragma unroll_loop",

" #pragma unroll_loop",

" #pragma unroll_loop",

@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 4, 2020

In another PR I can upgrade the rest of the shaders and the docs if this is accepted.

Please do this all in one PR if @mrdoob is fine with the new code. In this way, changes are nicely bundled.

Besides, does this PR replace #18762?

@Mugen87 Mugen87 changed the title Add #unroll_loop_start /end syntax WebGLRenderer: Add #unroll_loop_start /end syntax. Mar 4, 2020
@gkjohnson
Copy link
Collaborator Author

Please do this all in one PR if @mrdoob is fine with the new code. In this way, changes are nicely bundled.

The core shaders and example shaders all still have to be converted. I wanted to keep the initial PR small so it's easier to review and look at. It's not always clear to me when something will or won't get merged even when it's been mentioned in other threads so if I can save time by not writing docs and updating everything until it's clear everyone is happy with the approach I will. If this will definitely get merged I can update the remaining files in this PR. I'm just trying to save time for myself and you guys!

Besides, does this PR replace #18762?

Yes this should replace that PR.

@mrdoob
Copy link
Owner

mrdoob commented Mar 5, 2020

Actually, it may be good to continue using #pragma?

#pragma unroll_loop_start
for ( int i = 0; i < 5; i ++ ) {

  // nested blocks for days

}
#pragma unroll_loop_end

@gkjohnson
Copy link
Collaborator Author

@mrdoob I just changed the syntax to include #pragma. Let me know if it you're happy with it and I can update the docs and remaining shaders!

@mrdoob mrdoob added this to the r115 milestone Mar 6, 2020
@mrdoob mrdoob merged commit c634a75 into mrdoob:dev Mar 6, 2020
@mrdoob
Copy link
Owner

mrdoob commented Mar 6, 2020

Thanks!

@mrdoob mrdoob changed the title WebGLRenderer: Add #unroll_loop_start /end syntax. WebGLRenderer: Add #pragma unroll_loop_start/end syntax. Mar 6, 2020
@gkjohnson gkjohnson deleted the support-blocks-in-unrolled-loops branch June 1, 2020 21:10
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