Remove improper "active" attribute from skyBox in DotSceneLoader

[sercero]

This option is being used in DotSceneLoader.cpp but is absent in the DTD.

[paroj]

I think we should rather remove it from DotScene

[sercero]

Its strange the way it is done, on one hand setSkyBox() has an "enable" parameter.
On the other hand the DotSceneLoader bails if the SkyBox is not "active".
Perhaps we should maintain the "active" but turn it into the "enable".

ogre/PlugIns/DotScene/src/DotSceneLoader.cpp

Lines 779 to 789 in 6bcf1d7

	if (!active)
	return;
	// Process rotation (?)
	Quaternion rotation = Quaternion::IDENTITY;
	if (auto pElement = XMLNode.child("rotation"))
	rotation = parseQuaternion(pElement);
	// Setup the sky box
	mSceneMgr->setSkyBox(true, material, distance, drawFirst, rotation, m_sGroupName);

[paroj]

the current code comes from ogitor, so this might have been driven by some ogitor specific needs.

I would rather think about the use-case. Why would someone want to export a disabled skyBox? And is this a common enough usage to force handling of it to all importers/ exporters out there? (as they would have to follow the DTD)

[sercero]

I guess if you have a SkyBox in your scene you want to have it enabled 😂.

The issue is that enabled/disabled is an actual parameter of setSkyBox().

So one might argue that people would want to choose from the xml if the skybox is enabled or not.

I'm not sure what the use case would be to be honest.

[paroj]

The issue is that enabled/disabled is an actual parameter of setSkyBox().

this parameter is there so you can call setSkyBox(false) to disable it, which is not relevant for .scene I would say

[sercero]

The issue is that enabled/disabled is an actual parameter of setSkyBox().

this parameter is there so you can call setSkyBox(false) to disable it, which is not relevant for .scene I would say

Perhaps someone would want to set the SkyBox but have it disabled, and then call setSkyBox(enable) at some point in the code.

I think that the "active" or "enable" option should be in the DTD and also the behaviour of DotSceneLoader should also be changed so that it passes this option to the setSkyBox(...)

That way the DTD reflects better the OGRE API...

[paroj]

That way the DTD reflects better the OGRE API...

I dont think that this should be a goal of the file format. For one, I am not really happy with the current "sky" API.
Internally there are now SkyRenderers like:

ogre/OgreMain/src/OgreSceneManager.cpp

Lines 1190 to 1195 in a98c9ee

	void SceneManager::_setSkyBox(bool enable, const String& materialName, Real distance,
	uint8 renderQueue, const Quaternion& orientation,
	const String& groupName)
	{
	mSkyBox.setSkyBox(enable, materialName, distance, renderQueue, orientation, groupName);
	}

However I do not find them convenient enough to break the old API. But when it comes to it, it is much easier to break API then changing the file format.

Furthermore, .scene is also used by the jMonkey engine, so it should not be specific to the current Ogre API:
https://wiki.jmonkeyengine.org/docs/3.4/core/asset/asset_manager.html#example-code-loading-assets

[sercero]

That way the DTD reflects better the OGRE API...
OK, forget I said this.

But the other point still remains.

I think that there is a use case for having the SkyBox disabled in the .scene and enabling it later.

[paroj]

this can be handled by disabling the skybox immediately after loading. No need to extend the file format for that.

As I said, removing stuff from a file format is really hard, so I prefer to keep things minimal here.