Page MenuHome

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

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

Details

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

Event Timeline

Neal Alexander (NHA) retitled this revision from to Fix: Blender/GPU - GPU_shader_export fails to initialize lamp data for certain lamp parameters..May 18 2015, 8:25 PM
Neal Alexander (NHA) updated this object.
Neal Alexander (NHA) set the repository for this revision to rB Blender.
Neal Alexander (NHA) updated this revision to Diff 4255.
Neal Alexander (NHA) removed rB Blender as the repository for this revision.
Neal Alexander (NHA) updated this revision to Diff 4263.

Diffed against master branch instead of 2.74.

Campbell Barton (campbellbarton) requested changes to this revision.
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
Antony Riakiotakis (psy-fi) requested changes to this revision.
Antony Riakiotakis (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)

Neal Alexander (NHA) updated this revision to Diff 4267.

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.

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

Neal Alexander (NHA) updated this revision to Diff 4268.
  • Added missing constants to gpu.c.
  • Moved #defines out of enum.
Antony Riakiotakis (psy-fi) accepted this revision.

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.

Campbell Barton (campbellbarton) accepted this revision.

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