[Mesa-dev] [PATCH 10/14] mesa/main: Maintain compressed fog mode.

[Gustaw Smolarczyk]

2017-04-08 18:37 GMT+02:00 Marek Olšák <maraeo at gmail.com>:
> On Sat, Apr 8, 2017 at 12:53 AM, Gustaw Smolarczyk <wielkiegie at gmail.com> wrote:
>> 2017-04-07 23:56 GMT+02:00 Marek Olšák <maraeo at gmail.com>:
>>> On Fri, Apr 7, 2017 at 6:54 PM, Gustaw Smolarczyk <wielkiegie at gmail.com> wrote:
>>>> 2017-04-07 16:31 GMT+02:00 Marek Olšák <maraeo at gmail.com>:
>>>>> On Thu, Mar 30, 2017 at 8:09 PM, Gustaw Smolarczyk <wielkiegie at gmail.com> wrote:
>>>>>> Signed-off-by: Gustaw Smolarczyk <wielkiegie at gmail.com>
>>>>>> ---
>>>>>>  src/mesa/main/enable.c |  1 +
>>>>>>  src/mesa/main/fog.c    |  9 +++++++++
>>>>>>  src/mesa/main/mtypes.h | 14 ++++++++++++++
>>>>>>  3 files changed, 24 insertions(+)
>>>>>>
>>>>>> diff --git a/src/mesa/main/enable.c b/src/mesa/main/enable.c
>>>>>> index d9d63a6b4b..ef278a318a 100644
>>>>>> --- a/src/mesa/main/enable.c
>>>>>> +++ b/src/mesa/main/enable.c
>>>>>> @@ -385,6 +385,7 @@ _mesa_set_enable(struct gl_context *ctx, GLenum cap, GLboolean state)
>>>>>>              return;
>>>>>>           FLUSH_VERTICES(ctx, _NEW_FOG);
>>>>>>           ctx->Fog.Enabled = state;
>>>>>> +         ctx->Fog._PackedEnabledMode = state ? ctx->Fog._PackedMode : FOG_NONE;
>>>>>>           break;
>>>>>>        case GL_LIGHT0:
>>>>>>        case GL_LIGHT1:
>>>>>> diff --git a/src/mesa/main/fog.c b/src/mesa/main/fog.c
>>>>>> index 1ad939cfde..76e65080b7 100644
>>>>>> --- a/src/mesa/main/fog.c
>>>>>> +++ b/src/mesa/main/fog.c
>>>>>> @@ -102,8 +102,13 @@ _mesa_Fogfv( GLenum pname, const GLfloat *params )
>>>>>>           m = (GLenum) (GLint) *params;
>>>>>>          switch (m) {
>>>>>>          case GL_LINEAR:
>>>>>> +           ctx->Fog._PackedMode = FOG_LINEAR;
>>>>>> +           break;
>>>>>>          case GL_EXP:
>>>>>> +           ctx->Fog._PackedMode = FOG_EXP;
>>>>>> +           break;
>>>>>>          case GL_EXP2:
>>>>>> +           ctx->Fog._PackedMode = FOG_EXP2;
>>>>>
>>>>> Perhaps these should be set before FLUSH_VERTICES?
>>>>>
>>>>> Marek
>>>>
>>>> That would make us need two switch() statements instead of one. Also,
>>>> if ctx->Fog.Mode == m then we are essentially writing the same values
>>>> so nothing changes - doing it after the check shouldn't affect
>>>> correctness in any way. I might be wrong, though. _PackedMode is only
>>>> ever used to manage _PackedEnabledMode (here and in _mesa_set_enable),
>>>> so I think we should be safe.
>>>>
>>>> Since this simplification is pretty minor, I can change the code if you want.
>>>
>>> It's OK if _PackedMode is only a temporary variable used after
>>> FLUSH_VERTICES. It can't be used directly in places that don't call
>>> FLUSH_VERTICES though (e.g. ff_fragment_program.cpp).
>>
>> Since current _PackedMode usage is restricted to mesa/main just after
>> FLUSH_VERTICES is called (and will probably stay this way), I think
>> the prerequisites are fulfilled.
>
> The prerequisites are fulfilled only if _PackedMode isn't used by
> FLUSH_VERTICES itself.
>
> Mesa doesn't send all draw calls to drivers. Some draw calls are
> queued in the vbo module. FLUSH_VERTICES flushes the queue. All state
> changes should happen after FLUSH_VERTICES. If they happen before
> FLUSH_VERTICES, they could affect previous draw calls. It's basically
> the same as changing the order of GL calls in an incorrect way. We
> should make sure that it never happens.

Yes, I understand how it's important not to modify some state before
the driver (or vbo module or st/mesa) is done using it for things that
were being buffered (like the vertices). Since currently _PackedMode
is not used there, it shouldn't be a problem. And I think it's always
better to use _PackedEnabledMode instead of _PackedMode since it also
tells you whether the whole fog processing is enabled or not. Maybe I
should put a comment saying that _PackedMode is for internal use and
shouldn't be used in any other way than helping _PackedEnabledMode
initialization?

>
>>
>> If you are ok with the patches, could please push the whole series? I
>> don't have commit access.
>
> If you send me a git link to the whole series, I'll push it.

I am not sure what you mean by "git link". Did I forget about
something while using git-format-patch/git-send-email before sending
the series? Or do you want a git repository you can pull the series
from?

Gustaw