Normals calculation for terrain ignores scaling

[Rulatir]

To reproduce:

1) addTerrainSceneNode with non-flat terrain but set the y axis scale to 0.0001 to make it nearly flat; also set color to white.

2) addLightSceneNode to light the terrain.

Expected behavior: no terrain features should be discernable from most POVs since the final geometry after scaling is almost flat.

Observed behavior: very contrasting highlights and shades of the terrain features can be seen on the nearly-flat square.

Analysis: computation of normals ignores the scale factor.

Workaround: none found so far. Note that this has nothing to do with EMF_NORMALIZE_NORMALS. The normals probably are normalized (i.e. squares of their coordinates sum to 1), it's just that their orientations are wrong.

[Rulatir]

Test case:

Code: Select all

#include <irrlicht.h>
#include <iostream>
#include <GL/gl.h>

using namespace irr;

using namespace core;
using namespace scene;
using namespace video;
using namespace io;
using namespace gui;

//press space to toggle EMF_NORMALIZE_NORMALS for terrain
class MyEventReceiver : public IEventReceiver {

private:

    ITerrainSceneNode* m_terrain;
    
public:

    MyEventReceiver(ITerrainSceneNode* terrain) : m_terrain(terrain) {}
    
    virtual bool OnEvent(const irr::SEvent& event)
    {
        if
            (   event.EventType == EET_KEY_INPUT_EVENT
            &&  event.KeyInput.Char==L' '
            &&  event.KeyInput.PressedDown
            )
        {
            m_terrain->setMaterialFlag(
            
                EMF_NORMALIZE_NORMALS,
                !m_terrain->getMaterial(0).getFlag(EMF_NORMALIZE_NORMALS)
            );
            
            std::cout<<"EMF_NORMALIZE_NORMALS toggled for terrain.\n";
            return true;
        }
        
        return false;
    }
};

int main(int argc, char** argv)
{

    IrrlichtDevice *device =
        createDevice(EDT_OPENGL, dimension2d<s32>(800, 600), 32,
            false, false, false, 0);

    device->setWindowCaption(
        L"Hello Wrong Terrain Normals! - Irrlicht engine test case"
    );

    IVideoDriver* driver = device->getVideoDriver();
    ISceneManager* smgr = device->getSceneManager();
    IGUIEnvironment* guienv = device->getGUIEnvironment();

    smgr->setAmbientLight(SColorf(0.125,0.125,0.125));
    
    ITerrainSceneNode* terrain=smgr->addTerrainSceneNode(
        
        "../../media/terrain-heightmap.bmp",
        0, 1025,
        vector3df(0.0,0.0,0.0), vector3df(0.0,0.0,0.0),
        vector3df(1.0,0.0001,1.0), SColor(0xFFFFFFFF)
    );
    
    if (!terrain) return EXIT_FAILURE;
    
    terrain->setMaterialFlag(EMF_GOURAUD_SHADING, true);
    terrain->setMaterialFlag(EMF_LIGHTING, true);
    
    
    ICameraSceneNode* cam=smgr->addCameraSceneNodeFPS();
    float tX=terrain->getBoundingBox().getExtent().X;
    cam->setPosition(vector3df(0.0, tX/2, 0.0));
    cam->setTarget(terrain->getTerrainCenter());

    //bind light to camera
    ILightSceneNode* light
        = smgr->addLightSceneNode(
            cam, vector3df(0,0,0), SColorf(0.125,0.125,0.125),8.0
    );

    device->setEventReceiver(new MyEventReceiver(terrain));

    while(device->run())
    {
        driver->beginScene(true, true, SColor(0,200,200,200));

        smgr->drawAll();

        driver->endScene();
    }

    device->drop();

    return 0;
}

[rogerborg]

I don't have a 1.4 / 1.4.1 checkout, but I think this might do what you want for the SVN trunk.

It will change the normals for all (non identity scaled) terrains, so I'm not proposing it as a candidate, just as something that you might try to see if it fixes your problem.

Code: Select all

Index: source/Irrlicht/CTerrainSceneNode.cpp
===================================================================
--- source/Irrlicht/CTerrainSceneNode.cpp	(revision 1510)
+++ source/Irrlicht/CTerrainSceneNode.cpp	(working copy)
@@ -931,18 +931,18 @@
 				// top left
 				if (x>0 && z>0)
 				{
-					a = mb->Vertices[(x-1)*TerrainData.Size+z-1].Pos;
-					b = mb->Vertices[(x-1)*TerrainData.Size+z].Pos;
-					c = mb->Vertices[x*TerrainData.Size+z].Pos;
+					a = mb->Vertices[(x-1)*TerrainData.Size+z-1].Pos * TerrainData.Scale;
+					b = mb->Vertices[(x-1)*TerrainData.Size+z].Pos * TerrainData.Scale;
+					c = mb->Vertices[x*TerrainData.Size+z].Pos * TerrainData.Scale;
 					b -= a;
 					c -= a;
 					t = b.crossProduct ( c );
 					t.normalize ( );
 					normal += t;
 
-					a = mb->Vertices[(x-1)*TerrainData.Size+z-1].Pos;
-					b = mb->Vertices[x*TerrainData.Size+z-1].Pos;
-					c = mb->Vertices[x*TerrainData.Size+z].Pos;
+					a = mb->Vertices[(x-1)*TerrainData.Size+z-1].Pos * TerrainData.Scale;
+					b = mb->Vertices[x*TerrainData.Size+z-1].Pos * TerrainData.Scale;
+					c = mb->Vertices[x*TerrainData.Size+z].Pos * TerrainData.Scale;
 					b -= a;
 					c -= a;
 					t = b.crossProduct ( c );
@@ -955,18 +955,18 @@
 				// top right
 				if (x>0 && z<TerrainData.Size-1)
 				{
-					a = mb->Vertices[(x-1)*TerrainData.Size+z].Pos;
-					b = mb->Vertices[(x-1)*TerrainData.Size+z+1].Pos;
-					c = mb->Vertices[x*TerrainData.Size+z+1].Pos;
+					a = mb->Vertices[(x-1)*TerrainData.Size+z].Pos * TerrainData.Scale;
+					b = mb->Vertices[(x-1)*TerrainData.Size+z+1].Pos * TerrainData.Scale;
+					c = mb->Vertices[x*TerrainData.Size+z+1].Pos * TerrainData.Scale;
 					b -= a;
 					c -= a;
 					t = b.crossProduct ( c );
 					t.normalize ( );
 					normal += t;
 
-					a = mb->Vertices[(x-1)*TerrainData.Size+z].Pos;
-					b = mb->Vertices[x*TerrainData.Size+z+1].Pos;
-					c = mb->Vertices[x*TerrainData.Size+z].Pos;
+					a = mb->Vertices[(x-1)*TerrainData.Size+z].Pos * TerrainData.Scale;
+					b = mb->Vertices[x*TerrainData.Size+z+1].Pos * TerrainData.Scale;
+					c = mb->Vertices[x*TerrainData.Size+z].Pos * TerrainData.Scale;
 					b -= a;
 					c -= a;
 					t = b.crossProduct ( c );
@@ -979,18 +979,18 @@
 				// bottom right
 				if (x<TerrainData.Size-1 && z<TerrainData.Size-1)
 				{
-					a = mb->Vertices[x*TerrainData.Size+z+1].Pos;
-					b = mb->Vertices[x*TerrainData.Size+z].Pos;
-					c = mb->Vertices[(x+1)*TerrainData.Size+z+1].Pos;
+					a = mb->Vertices[x*TerrainData.Size+z+1].Pos * TerrainData.Scale;
+					b = mb->Vertices[x*TerrainData.Size+z].Pos * TerrainData.Scale;
+					c = mb->Vertices[(x+1)*TerrainData.Size+z+1].Pos * TerrainData.Scale;
 					b -= a;
 					c -= a;
 					t = b.crossProduct ( c );
 					t.normalize ( );
 					normal += t;
 
-					a = mb->Vertices[x*TerrainData.Size+z+1].Pos;
-					b = mb->Vertices[(x+1)*TerrainData.Size+z+1].Pos;
-					c = mb->Vertices[(x+1)*TerrainData.Size+z].Pos;
+					a = mb->Vertices[x*TerrainData.Size+z+1].Pos * TerrainData.Scale;
+					b = mb->Vertices[(x+1)*TerrainData.Size+z+1].Pos * TerrainData.Scale;
+					c = mb->Vertices[(x+1)*TerrainData.Size+z].Pos * TerrainData.Scale;
 					b -= a;
 					c -= a;
 					t = b.crossProduct ( c );
@@ -1003,18 +1003,18 @@
 				// bottom left
 				if (x<TerrainData.Size-1 && z>0)
 				{
-					a = mb->Vertices[x*TerrainData.Size+z-1].Pos;
-					b = mb->Vertices[x*TerrainData.Size+z].Pos;
-					c = mb->Vertices[(x+1)*TerrainData.Size+z].Pos;
+					a = mb->Vertices[x*TerrainData.Size+z-1].Pos * TerrainData.Scale;
+					b = mb->Vertices[x*TerrainData.Size+z].Pos * TerrainData.Scale;
+					c = mb->Vertices[(x+1)*TerrainData.Size+z].Pos * TerrainData.Scale;
 					b -= a;
 					c -= a;
 					t = b.crossProduct ( c );
 					t.normalize ( );
 					normal += t;
 
-					a = mb->Vertices[x*TerrainData.Size+z-1].Pos;
-					b = mb->Vertices[(x+1)*TerrainData.Size+z].Pos;
-					c = mb->Vertices[(x+1)*TerrainData.Size+z-1].Pos;
+					a = mb->Vertices[x*TerrainData.Size+z-1].Pos * TerrainData.Scale;
+					b = mb->Vertices[(x+1)*TerrainData.Size+z].Pos * TerrainData.Scale;
+					c = mb->Vertices[(x+1)*TerrainData.Size+z-1].Pos * TerrainData.Scale;
 					b -= a;
 					c -= a;
 					t = b.crossProduct ( c );

[Rulatir]

I don't have a 1.4 / 1.4.1 checkout, but I think this might do what you want for the SVN trunk.

It will change the normals for all (non identity scaled) terrains, so I'm not proposing it as a candidate, just as something that you might try to see if it fixes your problem.

Just by looking at your code, it seems to do what I want - it applies scale to vertices before computing normals from them. Just one quick question - is this code invoked from CTerrainSceneNode::setScale() so it will recalculate normals every time the scale changes?

[rogerborg]

Not at the moment. However, we could easily add a call to calculateNormals() to CTerrainSceneNode::setScale(), after getting the required mesh buffer(s) from Mesh.getMeshBuffer().

[Rulatir]

Rogerborg, for the time being, do you confirm this is a bug? Do you agree that the current implementation doesn't do the right thing?

[rogerborg]

I think that it would be useful to change the behaviour; I'm just concerned that there may be users currently depending on the existing normal calculation.

[Rulatir]

I think that it would be useful to change the behaviour; I'm just concerned that there may be users currently depending on the existing normal calculation.

Why not a choice defaulting to old behavior?

The solution below is a hack but that's the penalty for implementing an incorrect formula in the first place. The runtime cost is in fact negligible, especially if you don't use the feature.

Code: Select all

class CTerrainSceneNode : public ITerrainSceneNode {

private:

	//adding a static member doesn't break binary compatibility
	static std::set<const CTerrainSceneNode*> sm_nodesWithAutoNormals;

public:

	//adding non-virtual functions doesn't break binary compatibility
	bool getAutoNormalsMode() const {
		
		return
			sm_nodesWithAutoNormals.find(this)
			!= sm_nodesWithAutoNormals.end();
	}

	void setAutoNormalsMode(bool value) {

		if (value)
			sm_nodesWithAutoNormals.insert(this);
		else
			sm_nodesWithAutoNormals.erase(this);
	}
	
	virtual ~CTerrainSceneNode() {
		
		//...
		setAutoNormalsMode(false);
		//...
	}

private:

	foo /* I don't know the return type */ calculateNormals() {

		vector3df scaleForNormals
			= getAutoNormalsMode()
				? TerrainData.Scale
				: vector3df(1.0, 1.0, 1.0);

		//proceed as in your patch but using scaleForNormals
	}
}

We can afford a single std::set lookup per call to calculateNormals because we are doing lots of calculations there anyway.

[Munger]

This isn't a bug. Perhaps it isn't the behavior you want, but it is consistent behavior and it can be very useful.

If normals follow any transformations, the logical extension would be to recalculate the normals after each transformation (translation, rotation or in your case scale). That would be incredibly inefficient. The idea with the current implementation is that transformations are applied to existing meshes - the transformations are render-time modifiers.

What you are trying to accomplish can be done by modifying the actual contents of the mesh before you generate the normals. Try something like this:

Code: Select all

IMeshManipulator *meshManip = sceneManager->getMeshManipulator();
meshManip->scaleMesh(terrainSceneNode->getMesh(), vector3df(1.0f, 0.00001f, 1.0f));
meshManip->recalculateNormals(terrainSceneNode->getMesh());

I haven't tried the above code, but I think you get the idea at least. And why are you using a terrain object if you don't want to model terrain? Why not just construct the mesh yourself and provide your own normals? If it's flat, you're not making good use of the terrain node (e.g. wasted triangles, LOD overheads). What are you trying to achieve?

[rogerborg]

And why are you using a terrain object if you don't want to model terrain?

See, now that's the question that I should have asked.

[Rulatir]

This isn't a bug. Perhaps it isn't the behavior you want, but it is consistent behavior and it can be very useful.

Can you suggest an example of its usefulness?

If normals follow any transformations, the logical extension would be to recalculate the normals after each transformation (translation, rotation or in your case scale).

Backends automatically rotate normals just fine and neither translations nor uniform scaling change orientations of faces (which is what normals represent), so in reality you would only have to recalculate normals for transformations that are not similarities.

Thanks for the IMeshManipulator hint, I'll try it.

And why are you using a terrain object if you don't want to model terrain? Why not just construct the mesh yourself and provide your own normals? If it's flat, you're not making good use of the terrain node

I only made it flat for the purpose of demonstrating the problem. I want to model terrain and I still need some vertical scaling, like 0.2, not 0.00001. For example if you load terrain-heightmap.bmp from the SDK's media folder and scale it uniformly, it ends up very "steep". One luminance level seems to be mapped to the same unit of length on the Y axis as one bitmap pixel on the X and Z axes. I didn't want to scale the bitmap because of quantization.

[Rulatir]

Code: Select all

IMeshManipulator *meshManip = sceneManager->getMeshManipulator();
meshManip->scaleMesh(terrainSceneNode->getMesh(), vector3df(1.0f, 0.00001f, 1.0f));
meshManip->recalculateNormals(terrainSceneNode->getMesh());

Tried it. It has no effect whatsoever, neither on the normals nor even on the geometry. Weird!

Adding this afterwards:

Code: Select all

terrainSceneNode->setScale(vector3df(1.0,1.0,1.0));

seems to "flush" the geometry, but normals are still wrong. Even weirder!

Devs? How do we manipulate terrain geometry after the terrain scene node has been created? Can it be done at all? If not, how do we achieve the goal of loading terrain from heightmap with vertical scaling applied and normals reflecting that? As things are, it seems completely impossible, and I would argue that it is a very basic use case.

[Frank Dodd]

This is an issue for myself too, however in my case with rotating terrains. I am struggling my way through the code and will post up any solution I come up with.

Edit:

Code: Select all

        calculateNormals( &RenderBuffer );

before

Code: Select all

	calculateDistanceThresholds( true );
	calculatePatchData();

in

Code: Select all

        CTerrainSceneNode::applyTransformation()

has worked for me.

[Rulatir]

In CTerrainSceneNode::applyTransformation()? You mean modifying Irrlicht code?

[Frank Dodd]

Yes this function in the CTerrainSceneNode.cpp file in the Irrlicht Source code and rebuilding it to produce your own DLL.

This has worked well for me although there does appear to be some misscalculation in the normal for the vertex at 0,0 that is a minor inconvenience.