⚙ D1414 World textures displaying for viewport in BI.

Dontsov Valentin (valentin_b4w) updated this revision to Diff 4694.Jul 17 2015, 6:59 PM

Dontsov Valentin (valentin_b4w) retitled this revision from to World textures displaying for viewport in BI..

Dontsov Valentin (valentin_b4w) updated this object.

Dontsov Valentin (valentin_b4w) added reviewers: Antonis Ryakiotakis (psy-fi), Sergey Sharybin (sergey).

Dontsov Valentin (valentin_b4w) set the repository for this revision to rB Blender.

Dontsov Valentin (valentin_b4w) added projects: Render Pipeline, GPU / Viewport.

Dontsov Valentin (valentin_b4w) added a parent revision: D1413: Cube map support for GLSL render..

Dontsov Valentin (valentin_b4w) added subscribers: Evgeny_Rodygin, Alexander Kovelenov (AlexKowel), Yuri Kovelenov (yurikovelenov), Alexander Romanov (a.romanov).

Some general feedback, glsl part i'll leave to @Antonis Ryakiotakis (psy-fi) to doublecheck. maybe some code there could be deduplicated.

source/blender/gpu/shaders/gpu_shader_material.glsl
1637	`wo_` is a weird prefix, prefer to have `world_`
source/blender/render/intern/source/pixelshading.c
514	Why this was changed and did you check it does not introduce any regressions?

Dontsov Valentin (valentin_b4w) mentioned this in D1413: Cube map support for GLSL render..Jul 23 2015, 2:30 PM

Renamed glsl functions.

Alexander Romanov (a.romanov) commandeered this revision.Sep 2 2015, 10:26 AM

Alexander Romanov (a.romanov) added a reviewer: Dontsov Valentin (valentin_b4w).

I have a suggestion to merge here D1413 for more comfortable review. @Antonis Ryakiotakis (psy-fi), @Sergey Sharybin (sergey) what do you think?

Merged with D1413.

Alexander Romanov (a.romanov) removed a parent revision: D1413: Cube map support for GLSL render..Jan 11 2016, 4:41 PM

Alexander Romanov (a.romanov) mentioned this in D1729: BI "Real Sky" option works incorrectly for environment texture..Jan 12 2016, 1:28 PM

Alexander Romanov (a.romanov) added a child revision: D1729: BI "Real Sky" option works incorrectly for environment texture..

Alexander Romanov (a.romanov) mentioned this in rB53711020396a: BI "Real Sky" option works incorrectly for environment texture..Jan 13 2016, 9:56 AM

fix blending

Real Sky, Blend Sky and Paper Sky options just terrible. There is no clear understanding how they should work and they works differently for viewport and BI, in some cases even not matching documentation for both engines. And perhaps even some of them mutually exclusive. Thus their work is left as is. In the future, the interface and behavior must be completely reviewed.
Now this patch is ready for review.

Alexander Romanov (a.romanov) updated this object.Jan 15 2016, 4:41 PM

Alexander Romanov (a.romanov) added a project: BF Blender: Next.Jan 18 2016, 10:55 AM

Antonis Ryakiotakis (psy-fi) removed a reviewer: Antonis Ryakiotakis (psy-fi).Jan 18 2016, 11:14 AM

Hi! Thanks very much for this patch! I used it to have cubemap reflections in the viewport (https://youtu.be/jbFFyU28Py8, http://www.pasteall.org/63559/diff (patch compatible with upbge sources. Surely needs clean ups and it's a bit buggy for the moment :)), http://www.pasteall.org/blend/40174)

Dontsov Valentin (valentin_b4w) edited edge metadata.Jan 19 2016, 1:11 AM

This comment was removed by Dontsov Valentin (valentin_b4w).

source/blender/render/intern/source/pixelshading.c
514	Possibly, this line should be moved to a separate patch. It changes sky blend factor according to the viewport and description of the "Blend Sky" flag ("Render background with natural progression from horizon to zenith"). Current implementation doesn't take a view direction into account.

Alexander Romanov (a.romanov) updated this object.Jan 19 2016, 6:59 AM

Alexander Romanov (a.romanov) edited edge metadata.

Hi! I didn't know that you wanted to include realtime cubemap reflections in the viewport. That's very cool :) About that, I asked HG1 about the little shader I wrote to make cubemap relections (taken from here: https://en.wikibooks.org/wiki/GLSL_Programming/Blender/Reflecting_Surfaces) and he made some remarks to optimize it: http://www.pasteall.org/63624 ... In the case you are interested... And there is also a bug with the little piece of code I made when you switch beetween solid/textured mode (The cubemap textures have to be unloaded and reloaded/the shader too after...).

I hope we'll see your patch very soon in the master. I didn't see a test file to test world textures... I tried but it didn't work for me (with my implementation for upbge (when I had do_world_tex in the patch) (I didn't know the correct setup). If you join a test file and a picture with the expected result, I could test (with Blender official sources) and tell you if all is right on my computer or if I find bugs.

Great job! Thanks

Alexander Romanov (a.romanov) updated this object.Jan 19 2016, 12:15 PM

In D1414#39089, @Ulysse Martin (youle) wrote:

(The cubemap textures have to be unloaded and reloaded/the shader too after...).

Blend Sky and similar options work in the solid mode. So, I think that disabling textures and shading for solid mode can be done in the future. I've attached example blend file.

Hi again. So I made a test of your patch. It's very very nice :)
I didn't know how to use the feature so I clicked everywhere and made a video of what I did. Here is the video: https://youtu.be/9zx-x1ll8VI

It seems there are few bugs.

For example, some buttons seems to not respond correctly. I guess this could be solved by replacing RNA_def_property_update(prop, 0, "rna_World_update"); with RNA_def_property_update(prop, NC_WORLD | ND_WORLD_DRAW, "rna_World_update"); for some properties or replacing rna_world_update with rna_world_draw_update (rna_world.c).

There's also a difference beetween solid mode and textured/material mode for angular texture (display is not the same). I don't know if it is caused by multiview or shading mode...

And the real sky button had no effect, I don't know if this is normal.

And maybe I forgot something. But it's a very impressive work. Congrats!

EDIT: tested on Windows 10 64 GTX970

In D1414#39094, @Ulysse Martin (youle) wrote:

Hi again. So I made a test of your patch. It's very very nice :)
I didn't know how to use the feature so I clicked everywhere and made a video of what I did. Here is the video: https://youtu.be/9zx-x1ll8VI

It seems there are few bugs.

Hi! Thanks for participating!
This patch is designed to affect only the texture and leave the rest as is.
I have already argued how "* Sky" options behave:
"Real Sky, Blend Sky and Paper Sky options just terrible. There is no clear understanding how they should work and they works differently for viewport and BI, in some cases even not matching documentation for both engines. And perhaps even some of them mutually exclusive. Thus their work is left as is. In the future, the interface and behavior must be completely reviewed."
As these changes are taking place in the 2.7 branch, the work in this direction should lead us to less regression. So I suggest to find what was broken by this patch. Then continue to work on all other shortcomings and solve it separately. And it is also important to find a regression in BI, not only Viewport. Note that the picture could be different for BI and Viewport. And this will be future work to find accordance between them.

Alexander Romanov (a.romanov) added a reviewer: Brecht Van Lommel (brecht).Jan 21 2016, 1:47 PM

fix Zen Up and Zen Down influence

Jacob Merrill (blueprintrandom) added a subscriber: Jacob Merrill (blueprintrandom).Jan 21 2016, 10:14 PM

Hi, I updated the patch for the last Blender sources if you are interested: http://www.pasteall.org/63728/diff (I hope I made no mistake, it took me a while :) )
I wanted to try some things with it. I'll edit my comment if I fix a bug or if I find something interesting.

Style fixes. Thanks, @Ulysse Martin (youle)

Majority of the changes are related on the cubemap support in gpu/. While i can't spot something bad in particular and don't see other way to do it, it still makes sense to pose some related guys here like @Mike Erwin (merwin) and @Campbell Barton (campbellbarton) perhaps.

For me patch seems rather fine.

source/blender/gpu/intern/gpu_draw.c
745 ↗	(On Diff #5896)	Broken indentation. I's also really preferred to have curly braces in such cases.
749 ↗	(On Diff #5896)	No need to calloc, you're setting all the elements anyway. Use malloc instead.
1108 ↗	(On Diff #5896)	It doesn't make much sense to have `BKE_image_has_bindcode` here -- it iterates all possible bind codes anyway in the average case, and later you're simply re-checking if particular bind code is available or not.
1131 ↗	(On Diff #5896)	Same as above.
source/blender/gpu/intern/gpu_texture.c
405 ↗	(On Diff #5896)	Camelcase shouldn't be used for variables names.

Mike Erwin (merwin) added inline comments.Jan 26 2016, 6:54 AM

source/blender/blenloader/intern/readfile.c
3642 ↗	(On Diff #5896)	Feel free to use C99 style here. for (int i = ... )
source/blender/gpu/intern/gpu_draw.c
736 ↗	(On Diff #5896)	Could use some ASCII art here to document which parts of the source rect map to TOP, FRONT, LEFT, etc.
741 ↗	(On Diff #5896)	Verify w == h since all cubemap faces must be square.
759 ↗	(On Diff #5896)	Should only need one memcpy per scanline for each cube face. So y loop is needed but x loop probably not.
809 ↗	(On Diff #5896)	Still need to check GPU_full_non_power_of_two_support().
836 ↗	(On Diff #5896)	Should be GL_RGBA16F_ARB here, from ARB_texture_float extension.
841 ↗	(On Diff #5896)	Looks like some recent OpenGL changes are overwritten by this patch. RGBA8 & RGBA16 in this case. Look through diffs carefully & change only what you intend.
887 ↗	(On Diff #5896)	RGBA16F_ARB and RGBA8

Corrected

response

source/blender/gpu/intern/gpu_draw.c
759 ↗	(On Diff #5896)	We should reverse lines in some cases. Therefore negative x and y occur.
1108 ↗	(On Diff #5896)	Here is a full piece of original code: if (ima->bindcode) { if (ima->tpageflag & IMA_MIPMAP_COMPLETE) { glBindTexture(GL_TEXTURE_2D, ima->bindcode); glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, gpu_get_mipmap_filter(0)); glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, gpu_get_mipmap_filter(1)); } else GPU_free_image(ima); } else ima->tpageflag &= ~IMA_MIPMAP_COMPLETE; To keep the logic I should reset the flag IMA_MIPMAP_COMPLETE, and I'm using BKE_image_has_bindcode here because 'if (ima->tpageflag & IMA_MIPMAP_COMPLETE)' branch exists indside "if (ima->bindcode)".
1131 ↗	(On Diff #5896)	I can correct the code here but I think that this code should look similar to the previous block

Mike Erwin (merwin) accepted this revision.Jan 27 2016, 2:42 AM

Mike Erwin (merwin) added a reviewer: Mike Erwin (merwin).

Mike Erwin (merwin) added inline comments.

source/blender/gpu/intern/gpu_draw.c
759 ↗	(On Diff #5903)	That makes sense. I didn't catch the reversing part.

This revision is now accepted and ready to land.Jan 27 2016, 2:42 AM

Closed by commit rB771f73b6bedb: World textures displaying for viewport in BI. (authored by Alexander Romanov (a.romanov)). · Explain WhyJan 27 2016, 10:10 AM

This revision was automatically updated to reflect the committed changes.

Congrats and thanks very much!

In D1414#39403, @Ulysse Martin (youle) wrote:

Congrats and thanks very much!

And thank you for the help!

Campbell Barton (campbellbarton) mentioned this in rB47498c4f05ee: Fix T47635: Texture paint performance regression.Mar 1 2016, 9:07 PM

Sergey Sharybin (sergey) mentioned this in rB322fa278296b: Fix T47635: Texture paint performance regression.Mar 4 2016, 5:36 PM

Porteries Tristan (panzergame) added a subscriber: Porteries Tristan (panzergame).Aug 5 2016, 8:54 PM

Porteries Tristan (panzergame) added inline comments.

source/blender/gpu/intern/gpu_texture.c
658 ↗	(On Diff #5909)	@Alexander Romanov (a.romanov) : What do in particular these both lines ? For me it creates an OpenGL error for multisamples textures as gl[Enable/Disable](GL_TEXTURE_2D_MULTISAMPLE) is invalid.

Alexander Romanov (a.romanov) added inline comments.Aug 8 2016, 1:34 PM

source/blender/gpu/intern/gpu_texture.c
658 ↗	(On Diff #5909)	It is just unbinding of the texture. How did you obtain this error? What is your system configuration?

Porteries Tristan (panzergame) added inline comments.Aug 8 2016, 2:11 PM

source/blender/gpu/intern/gpu_texture.c
658 ↗	(On Diff #5909)	I obtain this error only by running blender through a opengl debugger (e.g apitrace) else it cause nothing in the render. My system configuration is for opengl 3.3. But i don't think it matter a lot, i didn't see anything in the doc saying that GL_TEXTURE_2D_MULTISAMPLE is a valid enumeration for gl[Enable/Disable] : https://www.opengl.org/sdk/docs/man2/xhtml/glEnable.xml.

Alexander Romanov (a.romanov) mentioned this in D2145: Bugfix. glDisable with bad enum argument in GPU_texture_unbind.Aug 8 2016, 4:44 PM

Alexander Romanov (a.romanov) mentioned this in rB151390069a5b: Bugfix. glDisable with bad enum argument in GPU_texture_unbind.Aug 9 2016, 11:38 AM

World textures displaying for viewport in BI.
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 4737

source/blender/editors/space_view3d/view3d_draw.c

source/blender/gpu/intern/gpu_material.c

source/blender/gpu/shaders/gpu_shader_material.glsl

source/blender/python/intern/gpu.c

source/blender/render/intern/source/pixelshading.c

World textures displaying for viewport in BI.ClosedPublicActions