[2.8pre-1816] Incorrect Skybox Picker behavior with tag 0

[StrikerMan780]

Setting the tag for SkyPicker to 0 is supposed to make the sky for that sector use the map default, and not use any SkyBox things. See: [wiki]Classes:SkyPicker[/wiki]

Unfortunately, when ZDoom 2.7 was released, this broke. Now, it uses the first skybox thing with an ID of 0, which is incorrect.

I had once used this ability to have areas with a skybox, and others that use the normal map sky for some interesting effects (such as teleporting to different geological regions), but they no longer work.

This is a shot of the test map showing what it should look like: http://puu.sh/lcZwn/4db4c71dc2.png

As for what it shouldn't look like, here's the test map so you can see for yourself. http://zandronum.com/tracker/file_downl ... 4&type=bug

EDIT: It seems that the Skypicker code is the same between ZDoom 2.6 and 2.8, however, I think someone forgot to compensate for this change in the skybox viewpoint code itself:

2.8's code:

Code: Select all

void ASkyViewpoint::BeginPlay ()
{
	Super::BeginPlay ();

	if (tid == 0 && level.DefaultSkybox == NULL)
	{
		level.DefaultSkybox = this;
	}
}

2.6's code:

Code: Select all

void ASkyViewpoint::BeginPlay ()
{
	Super::BeginPlay ();

	if (tid == 0)
	{
		int i;

		for (i = 0; i <numsectors; i++)
		{
			if (sectors[i].FloorSkyBox == NULL)
			{
				sectors[i].FloorSkyBox = this;
			}
			if (sectors[i].CeilingSkyBox == NULL)
			{
				sectors[i].CeilingSkyBox = this;
			}
		}
	}
}

Is the ZDoom source compatible with Visual Studio 2015: CE? I really should get an environment set up where I can compile this.

[Graf Zahl]

Fixed. This actually required adding two new flags because with the default skybox moved to the global level data there was insufficient information to detect this case.

Regarding VS2015, sue you can use that, if you use CMake to generate the project. I'm using VS2013 but the procedure for 2015 is identical.

[VoidMage]

A note: code commited fails with gcc; the error says 'ASkyViewpoint *' and 'TObjPtr<ASkyViewpoint>.' aren't the same type.

[Graf Zahl]

GCC sucks. It's truly annoying how anal that compiler is about implicit type conversions.

[VoidMage]

Well, a fix is already commited, so it doesn't matter anymore, but I'd fix it slightly differently: change return type of GetSkyBox (and type of skybox in R_Subsector) to TObjPtr<ASkyViewpoint> and remove the cast of NULL (after the former change, gcc is OK with that).

On an unrelated note: you've flipped (probably accidentally) line endings in oal volume commit...
So, food for thought...

Of course, the tricky part is all contributors would need to care about this...

[Graf Zahl]

Autocrlf was probably the most idiotic thing Git ever invented, especially how it is implemented.
With SVN this was disabled for good reasons on ZDoom's repo. I have lost count how often a bad combination of Autocrlf, obnoxious editors and bad assumptions which are nearly typical when dealing with Unix-based software (as Git is) managed to mess up things.

I have no idea how those CRLFs constantly creep in again. Visual Studio doesn't do it automatically - I can normally work on a LF-only source base without ever running into issues.

Regarding TObjPtr, sorry, no, that'd be dead wrong. We do not want the TObjPtr semantics on a local variable, that'd be a very bad thing.

[VoidMage]

The way I recall it, Windows had its fair share of bad assumptions too - see notepad in Win9x/XP (it's been fixed quite awhile later).

[Graf Zahl]

Notepad is crap, that's why I do not use it, and do not care if files I create can be read with it.
I'm not talking about Linux/Unix making bad assumptions - I am talking about Linux devs making bad assumptions. That's fully ok for software that's not supposed to be used anywhere else, but for something as 'fixing' line endings something a lot more robust is needed than mutilating line endings on commit/checkout.

I often work on projects where I cannot afford this to have an impact, e.g. Android development on Windows, and yet Git promotes this blanket careless approach to 'fixing' the problem, but ultimately making it even worse.

[VoidMage]

OK, I might be slightly biased here, as the editor I most often use, while quite a few steps from a trivial one, doesn't do the automatic line ends conversion, so keeping track of those ^M gets tedious after awhile...

[Graf Zahl]

Wait, you use an editor that cannot handle different line endings? Those things still exist outside of Notepad? I'm shocked.

[VoidMage]

Well, to be clear, being a file editor is a secondary (or less) function of this program.
I'm talking about internal editor of Midnight Commander - warts and all, in most cases I use it for, it does suffice.

[Edward-san]

graf, can you port the gcc fix to gzdoom?

Also, just for info, clang, too, fails to compile in gzdoom, but with a different message:

Code: Select all

/home/edward-san/zdoom/gzdoom/trunk/src/p_sectors.cpp:809:78: error: 
      conditional expression is ambiguous; 'ASkyViewpoint *' can be converted to
      'TObjPtr<class ASkyViewpoint>' and vice versa
  ...& SECF_NOFLOORSKYBOX)? (ASkyViewpoint*)NULL : level.DefaultSkybox;
                          ^ ~~~~~~~~~~~~~~~~~~~~   ~~~~~~~~~~~~~~~~~~~
/home/edward-san/zdoom/gzdoom/trunk/src/p_sectors.cpp:813:84: error: 
      conditional expression is ambiguous; 'ASkyViewpoint *' can be converted to
      'TObjPtr<class ASkyViewpoint>' and vice versa
  ...& SECF_NOCEILINGSKYBOX)? (ASkyViewpoint*)NULL : level.DefaultSkybox;
                            ^ ~~~~~~~~~~~~~~~~~~~~   ~~~~~~~~~~~~~~~~~~~
2 errors generated.

[Graf Zahl]

Technically that makes at least more sense than what GCC does - but both never look at the resulting type, in this case the one that gets returned.
The GCC fix will be filtered down once I update the next time.