Paths

Table of Contentst

Fix: Blender/GPU - GPU_shader_export fails to initialize lamp data for certain lamp parameters.
AbandonedPublic
Actions

Authored by Campbell Barton (campbellbarton) on May 18 2015, 8:25 PM.

Details

Reviewers

Mike Erwin (merwin)
Neal Alexander (NHA)
Antonis Ryakiotakis (psy-fi)

Summary

Problem

GPU_shader_export() silently leaves uniform->lamp data undefined for:

GPU_DYNAMIC_LAMP_DISTANCE = 16
GPU_DYNAMIC_LAMP_ATT1 = 17
GPU_DYNAMIC_LAMP_ATT2 = 18
GPU_DYNAMIC_LAMP_SPOTSIZE = 19
GPU_DYNAMIC_LAMP_SPOTBLEND = 20

causing GLSL export scripts to be missing relevant information.

Underlying cause

GPU_shader_export tests if GPUDynamicType is a lamp by checking against the interval (GPU_DYNAMIC_LAMP_FIRST=6,GPU_DYNAMIC_LAMP_LAST=11), which is no longer sufficient.

The enum GPUDynamicType has unfortunately evolved two separate intervals for GPU_DYNAMIC_LAMP_*, which LAMP_FIRST/LAST no longer covers.

Fix
Since the original intent of GPU_DYNAMIC_LAMP_FIRST and GPU_DYNAMIC_LAMP_LAST has been lost. This patch (against master) replaces them with the macro test function GPU_DYNAMIC_IS_LAMP(GPUDynamicType).

Diff Detail

Repository: rB Blender

Event Timeline

Neal Alexander (NHA) updated this revision to Diff 4255.May 18 2015, 8:25 PM

Neal Alexander (NHA) retitled this revision from to Fix: Blender/GPU - GPU_shader_export fails to initialize lamp data for certain lamp parameters..

Neal Alexander (NHA) updated this object.

Neal Alexander (NHA) set the repository for this revision to rB Blender.

Neal Alexander (NHA) added projects: GPU / Viewport, Import/Export, Blender 2.70.

Neal Alexander (NHA) updated this object.May 18 2015, 8:28 PM

Diffed against master branch instead of 2.74.

@Antonis Ryakiotakis (psy-fi), guess you and @Mike Erwin (merwin) are best guys to check on this patch.

Campbell Barton (campbellbarton) requested changes to this revision.May 20 2015, 9:44 AM

Campbell Barton (campbellbarton) added a reviewer: Campbell Barton (campbellbarton).

Campbell Barton (campbellbarton) added a subscriber: Campbell Barton (campbellbarton).

Campbell Barton (campbellbarton) added inline comments.

source/blender/gpu/GPU_material.h
145–150	Why not just re-arrange these to be above `GPU_DYNAMIC_SAMPLER_2D***`.

This revision now requires changes to proceed.May 20 2015, 9:44 AM

Antonis Ryakiotakis (psy-fi) requested changes to this revision.May 20 2015, 11:20 AM

Antonis Ryakiotakis (psy-fi) edited edge metadata.

Antonis Ryakiotakis (psy-fi) added inline comments.

source/blender/gpu/GPU_material.h
150	Same as Campbell suggested. Those are runtime enums, we can rearrange without mishaps. I would not remove GPU_DYNAMIC_LAMP_FIRST and GPU_DYNAMIC_LAMP_LAST, they make code more readable.
169	Define is nice. It would read much nicer with proper GPU_DYNAMIC_LAMP_FIRST and GPU_DYNAMIC_LAMP_LAST. (also will be less complex after rearranging the enums)

In response to people wanting to shuffle enum values around:

I propose we just nuke the whole value range, for the following reasons:

The old method of using LAMP_FIRST/LAST already caused one bug.
The new method is more organized, flexible, maintainable, and allows for arbitrary group membership testing.
The old value range is excluded from the new range. Some scripts hard code against the old values, because the current python API is missing constants. These scripts will hard fail with the new values, instead of possibly introducing subtle bugs, due to the symbolic semantics changing.

Even in python these values would be used as runtime values. I would hardly expect anyone to store GPU_DYNAMIC_LAMP_DYNPERSMAT in a file for further reference. Instead, such values will likely be referenced during runtime and should always fetch the correct values.

Actually I like the new method! Just move those defines outside the enum declaration and it's ready for master.

In D1309#25857, @Antonis Ryakiotakis (psy-fi) wrote:

Even in python these values would be used as runtime values.

Regarding the third point in my previous post: several of the constants were completely missing from the python context, requiring people to hack around it by checking the C header, and re-defining it themselves in their script. The GPU python module is missing core types needed to determine what kind of uniform the export_shader function is referring to.

I would hardly expect anyone to store GPU_DYNAMIC_LAMP_DYNPERSMAT in a file for further reference. Instead, such values will likely be referenced during runtime and should always fetch the correct values.

Right now, the GPU module is not exporting these:

GPU_DYNAMIC_LAMP_ATT1
GPU_DYNAMIC_LAMP_ATT2
GPU_DYNAMIC_LAMP_DISTANCE
GPU_DYNAMIC_LAMP_SPOTBLEND
GPU_DYNAMIC_LAMP_SPOTSIZE
GPU_DYNAMIC_MAT_ALPHA
GPU_DYNAMIC_MAT_AMB
GPU_DYNAMIC_MAT_DIFFRGB
GPU_DYNAMIC_MAT_EMIT
GPU_DYNAMIC_MAT_HARD
GPU_DYNAMIC_MAT_REF
GPU_DYNAMIC_MAT_SPEC
GPU_DYNAMIC_MAT_SPECRGB

Should i add the missing constants to ./source/blender/python/intern/gpu.c in this patch or save it for another?

No problem, these can be added easily.

If you do add them it's OK to do as part of this patch as well

Added missing constants to gpu.c.
Moved #defines out of enum.

Looks good. I will need your email to add you as an author to the commit in git. Alternatively I can just commit it using my name and mention you as an author in the commit message.

Neal Alexander (NHA) added a comment.May 20 2015, 9:22 PM

This comment was removed by Campbell Barton (campbellbarton).

Campbell Barton (campbellbarton) mentioned this in rB2b0613b948c2: Fix: GPU_shader_export fails /w some lamp attrs.May 21 2015, 12:50 AM

Thanks for the updates, pushed rB2b0613b948c2de2d31044ee4f8f82463732b1f15

note: asking for mails as author isn't so precise, since name+mail with some minor difference comes up as a different author.
also posting email addr on public site isn't so nice.
Patches uploaded using arc will include author info.

This revision is now accepted and ready to land.May 21 2015, 12:52 AM

Campbell Barton (campbellbarton) commandeered this revision.May 21 2015, 12:53 AM

Campbell Barton (campbellbarton) abandoned this revision.

Campbell Barton (campbellbarton) edited reviewers, added: Neal Alexander (NHA); removed: Campbell Barton (campbellbarton).