0002258: sv_forcegldefaults is broken

(0012406)
Edward-san (developer)
2015-05-26 09:41
edited on: 2015-05-26 10:02

I believe I found a fix for this.

[edit]Does this happen only with gl_light_ambient?

[edit2] Fixed the gl_light_ambient issue with this changeset.

(0012407)
unknownna (updater)
2015-05-26 10:10
edited on: 2015-05-26 10:11

According to the wiki, it should also happen with gl_lightmode and gl_fogmode.

Quote from Wiki
If this is enabled, some default OpenGL rendering options are enforced, i.e. clients render as if gl_lightmode == 3, gl_light_ambient == 20.0, and gl_fogmode == 1. Users can not change the light/brightness (excluding Gamma) in OpenGL once this is enabled.

(0012408)
Edward-san (developer)
2015-05-26 11:33
edited on: 2015-05-26 12:03

Quote
According to the wiki, it should also happen with gl_lightmode and gl_fogmode.

So the bug appears also with these two? If yes, which wad or map shows the most evident effects of these settings?

[edit] nvm, managed to find out with normal doom2.wad. Fixed with these other changesets.

(0012409)
Edward-san (developer)
2015-05-26 12:21

If this will be merged to 3.0 repo, I'd like to change that code for improvements to code style, etc.

(0012416)
Torr Samaho (administrator)
2015-05-26 17:37

Quote from Edward-san
If this will be merged to 3.0 repo, I'd like to change that code for improvements to code style, etc.

Please elaborate. Does this mean you are unhappy with your own patch? If so, why don't you submit a patch that uses the code style you want?

(0012418)
Edward-san (developer)
2015-05-26 18:52
edited on: 2015-05-26 18:52

I was thinking about replacing any instance of gl_light_ambient and gl_fogmode with gl_GetLightAmbient and gl_GetFogMode, like it's done with gl_lightmode which has gl_GetLightMode, and remove the OVERRIDE_FOGMODE_IF_NECESSARY macro. Doing this would interfere with 3.0, wouldn't it?

(0012420)
Edward-san (developer)
2015-05-26 20:17

Here's the cleanup approach.

(0012429)
Torr Samaho (administrator)
2015-05-27 06:16

The whole purpose of the define was to minimize delta to GZDoom's code. Your approach has a much higher delta and is thus more difficult to merge with GZDoom code base upgrades. Instead, I would actually rather go into the other direction and introduce new defines for the light stuff or extend the current define.

(0012430)
Edward-san (developer)
2015-05-27 09:46
edited on: 2015-05-27 12:15

But this way, when we'll upgrade the code, with my patch it could be possible to track the move of these cvar instances. This bug happened because these cvars were moved around and the custom code was not updated properly to keep account of this.

[edit] Anyways, Here's the way you suggested.

(0012471)
Torr Samaho (administrator)
2015-05-31 12:04

Neat idea with the general define, but the fact that one has to copy and paste the default value of the CVAR to where the define is called is not so nice yet. Can you try to get the default value from the CVAR automatically?

Also, the history entry in zandronum-history.txt should not contain things like " After the code upgrade". First, the entries are meant for non-tech-savvy users and should just reflect what has changed since the last version. Second, after a large upgrade like in 2.0, probably half of the fix entries in the next minor version would have to have this prepended.

(0012472)
Dusk (developer)
2015-05-31 14:36
edited on: 2015-05-31 14:37

Quote from Torr Samaho

Can you try to get the default value from the CVAR automatically?

FBaseCVar has GetGenericRepDefault for this purpose.

(0012473)
Edward-san (developer)
2015-05-31 15:01

Quote from Dusk

Quote from Torr Samaho

Can you try to get the default value from the CVAR automatically?

FBaseCVar has GetGenericRepDefault for this purpose.

I still need to pass the cvar type to get the value:

cvar.GetGenericRepDefault().Int

if the cvar is not Int, how should we deal with it?

(0012484)
Edward-san (developer)
2015-05-31 21:45

Managed to work out with this.

(0012547)
Torr Samaho (administrator)
2015-06-06 15:15

I left a comment on bitbucket.

(0012579)
Edward-san (developer)
2015-06-07 15:13

Fixed in the same pull request yesterday. Just forgot to 'review' this ticket.

(0012642)
Edward-san (developer)
2015-06-10 21:14
edited on: 2015-06-10 21:14

Here's a build with the fix:'https://www.dropbox.com/s/wnxdk66qp8p3e1e/zandronum-2.1-150606-2115-37376c5-windows.zip?dl=0 [^]'

(0012647)
unknownna (updater)
2015-06-11 07:53

I'm still able to toggle the fog mode (gl_fogmode) in that build, and the light mode (gl_lightmode) setting doesn't seem to change if you connect with gl_lightmode set to 0.

Connect to the server like this:

zandronum -iwad doom2.wad -connect localhost +vid_renderer 1 +gl_light_ambient 255 +gl_distfog 2048 +gl_fogmode 2 +gl_lightmode 0

And speaking of which, I wonder if we should enforce gl_distfog to the default value when sv_forcegldefaults is set to 1.

(0012648)
Edward-san (developer)
2015-06-11 10:55

Quote from unknownna
I'm still able to toggle the fog mode (gl_fogmode) in that build

Do you mean that you can still see differences in the fog with sv_forcegldefaults is 1? Is it there a map which shows clearly what are you describing? I can't reproduce in doom2 map01, for example.

(0012650)
unknownna (updater)
2015-06-11 12:47

Yes. If gl_distfog is set to a high value, you'll see it very clearly in MAP01.

(0012651)
Edward-san (developer)
2015-06-11 14:50
edited on: 2015-06-11 15:36

I see it now. Strangely, every piece of code which uses gl_fogmode should've been already taken care, so I tested 1.3 and noticed that there was still a difference between gl_fogmode 1 and 2, although in my case there were artifacts for value 2. gl_fogmode 0 and 1 behave the same. Can you reproduce it?

(0012652)
unknownna (updater)
2015-06-11 19:16

Yeah, I'm able to reproduce the gl_fogmode issue in 1.3 and 1.0 as well. However, your fix still doesn't take care of the issue where the client connects with gl_lightmode set to 0.

(0012654)
Edward-san (developer)
2015-06-11 22:33
edited on: 2015-06-11 23:30

Fixed with'https://bitbucket.org/crimsondusk/zandronum-sandbox-stable/branches/compare/crimsondusk/zandronum-sandbox-stable:9eda5bb5325a%0DTorr_Samaho/zandronum-stable:default [^]' . I'll get a build with it asap.

(0012655)
WaTaKiD (updater)
2015-06-12 00:16

at Edward-san's request:'https://www.dropbox.com/s/idl86hz3r3iho60/zandronum-2.1-150611-2305-9eda5bb-windows.zip?dl=0 [^]'

(0012656)
unknownna (updater)
2015-06-12 01:38

Thanks for the build, WaTaKiD! While it seems to have taken care of the gl_lightmode issue, the gl_fogmode issue is still there, i.e., I'm still able to toggle between gl_fogmode 1 and 2.

And I still maintain that we have to enforce gl_distfog to its default value as well since you're able to effectively turn all fog off by setting gl_distfog to 0.

(0012657)
Edward-san (developer)
2015-06-12 09:34

Quote from unknownna
the gl_fogmode issue is still there

this could be discussed in the next dev meeting, because this was already present in 1.3 and, until Torr says otherwise, this seems intentional. If we need to improve this, I suggest to force gl_fogmode to 1 also in case gl_fogmode is 2, because that is not supported in very old gpus.

Quote from unknownna
And I still maintain that we have to enforce gl_distfog to its default value as well since you're able to effectively turn all fog off by setting gl_distfog to 0.

Interesting, this must be discussed, too.

Anyways, are there tickets or issues which are 2.0 regressions other than this?

(0012725)
cobalt (updater)
2015-06-14 19:58

Issue addressed by commit 4419bed7747c: - Fixed: sv_forcegldefaults 1 stopped working properly for the gl clients (addresses 2258).
Committed by Edoardo Prezioso [edward-san] on Friday 12 June 2015 01:05:18

Changes in files:

docs/zandronum-history.txt | 3 ++-
src/d_main.cpp | 24 ++++++++++++------------
src/gl/data/gl_data.cpp | 12 ++++++++----
src/gl/gl_functions.h | 12 ++++++++++--
src/gl/models/gl_models.cpp | 6 +++++-
src/gl/renderer/gl_lightdata.cpp | 19 ++++---------------
src/gl/renderer/gl_renderstate.cpp | 6 ++++++
src/gl/scene/gl_walls_draw.cpp | 2 +-
src/gl/shaders/gl_shader.cpp | 8 ++++++++
9 files changed, 56 insertions(+), 36 deletions(-)

(0012735)
Edward-san (developer)
2015-06-15 19:35

Adapted the code to the 3.0 codebase with'https://bitbucket.org/crimsondusk/zandronum-sandbox/commits/0be3e866b1123015a19f9e631285447de728543b [^]' .

(0012761)
unknownna (updater)
2015-06-20 13:29

Torr, what's your verdict on gl_fogmode?

This issue is already marked as resolved. If you feel that is not the case, please reopen it and explain why.
Supporters:	No one explicitly supports this issue yet.
Opponents:	No one explicitly opposes this issue yet.

Issue History
Date Modified	Username	Field	Change
2015-05-26 05:12	unknownna	New Issue
2015-05-26 09:16	Edward-san	Note Added: 0012405
2015-05-26 09:16	Edward-san	Note Edited: 0012405	View Revisions
2015-05-26 09:17	Edward-san	Note Deleted: 0012405
2015-05-26 09:41	Edward-san	Note Added: 0012406
2015-05-26 09:41	Edward-san	Assigned To	=> Edward-san
2015-05-26 09:41	Edward-san	Status	new => assigned
2015-05-26 09:48	Edward-san	Note Edited: 0012406	View Revisions
2015-05-26 10:02	Edward-san	Note Edited: 0012406	View Revisions
2015-05-26 10:02	Edward-san	Status	assigned => needs review
2015-05-26 10:02	Edward-san	Target Version	=> 2.1
2015-05-26 10:10	unknownna	Note Added: 0012407
2015-05-26 10:11	unknownna	Note Edited: 0012407	View Revisions
2015-05-26 11:33	Edward-san	Note Added: 0012408
2015-05-26 11:39	Edward-san	Note Edited: 0012408	View Revisions
2015-05-26 11:40	Edward-san	Note Edited: 0012408	View Revisions
2015-05-26 12:03	Edward-san	Note Edited: 0012408	View Revisions
2015-05-26 12:21	Edward-san	Note Added: 0012409
2015-05-26 17:37	Torr Samaho	Note Added: 0012416
2015-05-26 17:44	Torr Samaho	Status	needs review => feedback
2015-05-26 18:52	Edward-san	Note Added: 0012418
2015-05-26 18:52	Edward-san	Note Edited: 0012418	View Revisions
2015-05-26 20:17	Edward-san	Note Added: 0012420
2015-05-26 20:29	Edward-san	Status	feedback => needs review
2015-05-27 06:16	Torr Samaho	Note Added: 0012429
2015-05-27 09:46	Edward-san	Note Added: 0012430
2015-05-27 09:46	Edward-san	Note Edited: 0012430	View Revisions
2015-05-27 12:15	Edward-san	Note Edited: 0012430	View Revisions
2015-05-31 12:04	Torr Samaho	Note Added: 0012471
2015-05-31 12:09	Torr Samaho	Status	needs review => feedback
2015-05-31 14:36	Dusk	Note Added: 0012472
2015-05-31 14:37	Dusk	Note Edited: 0012472	View Revisions
2015-05-31 15:01	Edward-san	Note Added: 0012473
2015-05-31 21:45	Edward-san	Note Added: 0012484
2015-05-31 21:45	Edward-san	Status	feedback => needs review
2015-06-06 15:15	Torr Samaho	Note Added: 0012547
2015-06-06 15:15	Torr Samaho	Status	needs review => feedback
2015-06-07 15:13	Edward-san	Note Added: 0012579
2015-06-07 15:13	Edward-san	Status	feedback => needs review
2015-06-10 21:14	Edward-san	Note Added: 0012642
2015-06-10 21:14	Edward-san	Note Edited: 0012642	View Revisions
2015-06-11 07:53	unknownna	Note Added: 0012647
2015-06-11 10:55	Edward-san	Note Added: 0012648
2015-06-11 12:47	unknownna	Note Added: 0012650
2015-06-11 14:50	Edward-san	Note Added: 0012651
2015-06-11 15:36	Edward-san	Note Edited: 0012651	View Revisions
2015-06-11 19:16	unknownna	Note Added: 0012652
2015-06-11 19:16	unknownna	Steps to Reproduce Updated	View Revisions
2015-06-11 22:33	Edward-san	Note Added: 0012654
2015-06-11 23:30	Edward-san	Note Edited: 0012654	View Revisions
2015-06-12 00:16	WaTaKiD	Note Added: 0012655
2015-06-12 01:38	unknownna	Note Added: 0012656
2015-06-12 09:34	Edward-san	Note Added: 0012657
2015-06-12 10:57	Dusk	Relationship added	related to 0001850
2015-06-14 19:58	cobalt	Status	needs review => needs testing
2015-06-14 19:58	cobalt	Steps to Reproduce Updated	View Revisions
2015-06-14 19:58	cobalt	Note Added: 0012725
2015-06-14 20:44	Edward-san	Relationship added	related to 0002312
2015-06-15 19:35	Edward-san	Note Added: 0012735
2015-06-20 13:29	unknownna	Note Added: 0012761
2015-06-24 22:45	Dusk	Status	needs testing => resolved
2015-06-24 22:45	Dusk	Fixed in Version	=> 2.2
2015-06-24 22:45	Dusk	Resolution	open => fixed
2015-06-24 22:48	Dusk	Fixed in Version	2.2 => 2.1
2018-09-30 21:37	Blzut3	Status	resolved => closed

Notes
(0012406) Edward-san (developer) 2015-05-26 09:41 edited on: 2015-05-26 10:02	I believe I found a fix for this. [edit]Does this happen only with gl_light_ambient? [edit2] Fixed the gl_light_ambient issue with this changeset.

(0012407) unknownna (updater) 2015-05-26 10:10 edited on: 2015-05-26 10:11	According to the wiki, it should also happen with gl_lightmode and gl_fogmode. Quote from Wiki If this is enabled, some default OpenGL rendering options are enforced, i.e. clients render as if gl_lightmode == 3, gl_light_ambient == 20.0, and gl_fogmode == 1. Users can not change the light/brightness (excluding Gamma) in OpenGL once this is enabled.

(0012408) Edward-san (developer) 2015-05-26 11:33 edited on: 2015-05-26 12:03	Quote According to the wiki, it should also happen with gl_lightmode and gl_fogmode. So the bug appears also with these two? If yes, which wad or map shows the most evident effects of these settings? [edit] nvm, managed to find out with normal doom2.wad. Fixed with these other changesets.

(0012409) Edward-san (developer) 2015-05-26 12:21	If this will be merged to 3.0 repo, I'd like to change that code for improvements to code style, etc.

(0012416) Torr Samaho (administrator) 2015-05-26 17:37	Quote from Edward-san If this will be merged to 3.0 repo, I'd like to change that code for improvements to code style, etc. Please elaborate. Does this mean you are unhappy with your own patch? If so, why don't you submit a patch that uses the code style you want?

(0012418) Edward-san (developer) 2015-05-26 18:52 edited on: 2015-05-26 18:52	I was thinking about replacing any instance of gl_light_ambient and gl_fogmode with gl_GetLightAmbient and gl_GetFogMode, like it's done with gl_lightmode which has gl_GetLightMode, and remove the OVERRIDE_FOGMODE_IF_NECESSARY macro. Doing this would interfere with 3.0, wouldn't it?

(0012420) Edward-san (developer) 2015-05-26 20:17	Here's the cleanup approach.

(0012429) Torr Samaho (administrator) 2015-05-27 06:16	The whole purpose of the define was to minimize delta to GZDoom's code. Your approach has a much higher delta and is thus more difficult to merge with GZDoom code base upgrades. Instead, I would actually rather go into the other direction and introduce new defines for the light stuff or extend the current define.

(0012430) Edward-san (developer) 2015-05-27 09:46 edited on: 2015-05-27 12:15	But this way, when we'll upgrade the code, with my patch it could be possible to track the move of these cvar instances. This bug happened because these cvars were moved around and the custom code was not updated properly to keep account of this. [edit] Anyways, Here's the way you suggested.

(0012471) Torr Samaho (administrator) 2015-05-31 12:04	Neat idea with the general define, but the fact that one has to copy and paste the default value of the CVAR to where the define is called is not so nice yet. Can you try to get the default value from the CVAR automatically? Also, the history entry in zandronum-history.txt should not contain things like " After the code upgrade". First, the entries are meant for non-tech-savvy users and should just reflect what has changed since the last version. Second, after a large upgrade like in 2.0, probably half of the fix entries in the next minor version would have to have this prepended.

(0012472) Dusk (developer) 2015-05-31 14:36 edited on: 2015-05-31 14:37	Quote from Torr Samaho Can you try to get the default value from the CVAR automatically? FBaseCVar has GetGenericRepDefault for this purpose.

(0012473) Edward-san (developer) 2015-05-31 15:01	Quote from Dusk Quote from Torr Samaho Can you try to get the default value from the CVAR automatically? FBaseCVar has GetGenericRepDefault for this purpose. I still need to pass the cvar type to get the value: cvar.GetGenericRepDefault().Int if the cvar is not Int, how should we deal with it?

(0012484) Edward-san (developer) 2015-05-31 21:45	Managed to work out with this.

(0012547) Torr Samaho (administrator) 2015-06-06 15:15	I left a comment on bitbucket.

(0012579) Edward-san (developer) 2015-06-07 15:13	Fixed in the same pull request yesterday. Just forgot to 'review' this ticket.

(0012642) Edward-san (developer) 2015-06-10 21:14 edited on: 2015-06-10 21:14	Here's a build with the fix:'https://www.dropbox.com/s/wnxdk66qp8p3e1e/zandronum-2.1-150606-2115-37376c5-windows.zip?dl=0 [^]'

(0012647) unknownna (updater) 2015-06-11 07:53	I'm still able to toggle the fog mode (gl_fogmode) in that build, and the light mode (gl_lightmode) setting doesn't seem to change if you connect with gl_lightmode set to 0. Connect to the server like this: zandronum -iwad doom2.wad -connect localhost +vid_renderer 1 +gl_light_ambient 255 +gl_distfog 2048 +gl_fogmode 2 +gl_lightmode 0 And speaking of which, I wonder if we should enforce gl_distfog to the default value when sv_forcegldefaults is set to 1.

(0012648) Edward-san (developer) 2015-06-11 10:55	Quote from unknownna I'm still able to toggle the fog mode (gl_fogmode) in that build Do you mean that you can still see differences in the fog with sv_forcegldefaults is 1? Is it there a map which shows clearly what are you describing? I can't reproduce in doom2 map01, for example.

(0012650) unknownna (updater) 2015-06-11 12:47	Yes. If gl_distfog is set to a high value, you'll see it very clearly in MAP01.

(0012651) Edward-san (developer) 2015-06-11 14:50 edited on: 2015-06-11 15:36	I see it now. Strangely, every piece of code which uses gl_fogmode should've been already taken care, so I tested 1.3 and noticed that there was still a difference between gl_fogmode 1 and 2, although in my case there were artifacts for value 2. gl_fogmode 0 and 1 behave the same. Can you reproduce it?

(0012652) unknownna (updater) 2015-06-11 19:16	Yeah, I'm able to reproduce the gl_fogmode issue in 1.3 and 1.0 as well. However, your fix still doesn't take care of the issue where the client connects with gl_lightmode set to 0.

(0012654) Edward-san (developer) 2015-06-11 22:33 edited on: 2015-06-11 23:30	Fixed with'https://bitbucket.org/crimsondusk/zandronum-sandbox-stable/branches/compare/crimsondusk/zandronum-sandbox-stable:9eda5bb5325a%0DTorr_Samaho/zandronum-stable:default [^]' . I'll get a build with it asap.

(0012655) WaTaKiD (updater) 2015-06-12 00:16	at Edward-san's request:'https://www.dropbox.com/s/idl86hz3r3iho60/zandronum-2.1-150611-2305-9eda5bb-windows.zip?dl=0 [^]'

(0012656) unknownna (updater) 2015-06-12 01:38	Thanks for the build, WaTaKiD! While it seems to have taken care of the gl_lightmode issue, the gl_fogmode issue is still there, i.e., I'm still able to toggle between gl_fogmode 1 and 2. And I still maintain that we have to enforce gl_distfog to its default value as well since you're able to effectively turn all fog off by setting gl_distfog to 0.

(0012657) Edward-san (developer) 2015-06-12 09:34	Quote from unknownna the gl_fogmode issue is still there this could be discussed in the next dev meeting, because this was already present in 1.3 and, until Torr says otherwise, this seems intentional. If we need to improve this, I suggest to force gl_fogmode to 1 also in case gl_fogmode is 2, because that is not supported in very old gpus. Quote from unknownna And I still maintain that we have to enforce gl_distfog to its default value as well since you're able to effectively turn all fog off by setting gl_distfog to 0. Interesting, this must be discussed, too. Anyways, are there tickets or issues which are 2.0 regressions other than this?

(0012725) cobalt (updater) 2015-06-14 19:58	Issue addressed by commit 4419bed7747c: - Fixed: sv_forcegldefaults 1 stopped working properly for the gl clients (addresses 2258). Committed by Edoardo Prezioso [edward-san] on Friday 12 June 2015 01:05:18 Changes in files: docs/zandronum-history.txt \| 3 ++- src/d_main.cpp \| 24 ++++++++++++------------ src/gl/data/gl_data.cpp \| 12 ++++++++---- src/gl/gl_functions.h \| 12 ++++++++++-- src/gl/models/gl_models.cpp \| 6 +++++- src/gl/renderer/gl_lightdata.cpp \| 19 ++++--------------- src/gl/renderer/gl_renderstate.cpp \| 6 ++++++ src/gl/scene/gl_walls_draw.cpp \| 2 +- src/gl/shaders/gl_shader.cpp \| 8 ++++++++ 9 files changed, 56 insertions(+), 36 deletions(-)

(0012735) Edward-san (developer) 2015-06-15 19:35	Adapted the code to the 3.0 codebase with'https://bitbucket.org/crimsondusk/zandronum-sandbox/commits/0be3e866b1123015a19f9e631285447de728543b [^]' .

(0012761) unknownna (updater) 2015-06-20 13:29	Torr, what's your verdict on gl_fogmode?