Page 22 of 28
Re: New Terrain Early Shots
Posted: Thu Feb 11, 2010 8:54 am
by stealth977
Below is the patch to supply functions to change WorldSize or MapSize of both Terrain and TerrainGroup, i tested them and worked fine (and alot easier/faster than doing explicitly), still you may want to review them since i may have added unnecessary steps to be sure
Re: New Terrain Early Shots
Posted: Fri Feb 19, 2010 10:30 am
by stealth977
First of all, i am requesting this feature since you create Ogre::Terrain as a final class (not a class to derive from) and making it possible to plug different components into it to customize it (like material generator).
The Feature Request:
What i request is you to strip serialization process from the class itself and provide it as a pluggable interface like the material generator.
The Reason:
Current binary format of the Terrain Data is not suitable for large scale deployment since it is not optimized. So if serializing can be pluggable, we can write our own serializers to save and load the data more efficiently (both time and space wise)
Re: New Terrain Early Shots
Posted: Fri Feb 19, 2010 11:49 am
by sinbad
That's a fair point. I'd intended to get back to optimisation of the serialized data, by allowing options to save as pre-scaled lower-precision data (16-bit) and also to add gzip compression wrapping to the DataStream classes. For example, a 12MB tile compresses to 3.5MB just using gzip, so deploying in a zip archive solves many of the issues simply. But you might be able to get better compression ratios by being more specific.
So, I'm open to the idea. If you're wanting to do this anyway, please submit a patch and we'll include it in 1.8.
Re: New Terrain Early Shots
Posted: Fri Feb 19, 2010 3:11 pm
by stealth977
The optimizations you mentioned are what i had on my mind while asking the serialization to be carried on by a pluggable class.
Due to high number of available options in case of serialization, its being pluggable would be more appropriate. Still a defaultserializer can supply some options for regular users, but the power lies in a pluggable interface.
Since explicit serialization is not an option (explicit here means fetching data from the terrain class and serializing out + using import interface during serializing in) due to high performance cost especially during loads, an abstract serializer interface would be a perfect solution.
Instead of:
stream->Write(COUNT);
stream->Write(SomeDataArrayContainingBlendmap)
you could use mCurrentSerializer->WriteBlendmap(BlendmapSize, BlendmapArrayPtr);
where WriteBlendMap is an abstract interface. So that the actual serializer can save it in whatever way it wants...Same for the loading process, you will just ask the serializer interface to return you whatever you want using predefined abstract interfaces...
So, user may select to output combinations of 32/16bit hmap, grayscale/full colour lightmaps, raw/rle encoded blendmaps and whatever else he likes, using the pluggable serializer interface...
Re: New Terrain Early Shots
Posted: Fri Feb 19, 2010 3:43 pm
by Praetor
I agree with the pluggable interface, both for saving and loading. Optimizations like you said might be handy, but I can provide them from my own set of classes (I suppose a compressed option on the StreamSerialiser would be handy). If we provide these hooks as additional interfaces then we should be able to include it in 1.7, right?
Re: New Terrain Early Shots
Posted: Fri Feb 19, 2010 4:08 pm
by sinbad
Praetor wrote:If we provide these hooks as additional interfaces then we should be able to include it in 1.7, right?
No, what's being proposed here is too significant a change IMO. It requires a significant refactoring of the 'prepare' implementation, and more direct access to many data internals to be able to inject the data more directly, neither of which is compatible with a stable release. If this was going to get changed for 1.7, it needed to be several weeks ago.
@stealth977: you have to bear in mind that the load routines are intended to be called in a background thread, and that data ownership / partitioning is very important. The save is the easy part (given access to the data to write out), but implementing the prepare() via a serialiser is a bit more complicated.
You can't just split off the methods and say loadBlendMaps() or something like that, because the way the (standard) serialisation is designed to work involves chunk-based data which is self-validating and also extensible (new versions of the file format can be introduced, optional chunks inserted etc). That works because it's pulled in as a stream, you can't randomly jump around it (well, you can actually skip forward but it's not efficient to do this and you can't skip backwards without restarting - you might recognise this concept as compatible with compression and encryption streams). You would need to implement any pluggable serialiser as a one-shot deal, a single prepare() or save() call. That in turn needs a way to retrieve pointers to all the data that's provided, including reflecting on optional parts etc.
I know you and some other people favour the random-access, pull data from fragmented sources approach for terrain, but having used that style before I don't like it. A packed and extensible format is overall much better since it is self-contained, consistent and you always know what you're dealing with, and is also more efficient to serialise because it's one big stream. It's also better for streaming off optical media, and like I say plays nice with compression and encryption. Personally I think simply having an option to gzip the stream and to encode the data within the packed format differently would be far, far simpler and ultimately more efficient than what you're proposing. I'm happy to still allow pluggable serialisers, but they'd have to work on the same basis - one single function call for load/save.
Re: New Terrain Early Shots
Posted: Fri Feb 19, 2010 4:26 pm
by stealth977
Just to clarify what i am trying to tell:
A Typical read sequence:
mCustomSerializer->open(...)
mCustomSerializer->readHeightMap(...)
mCustomSerializer->readBlendMaps(...)
mCustomSerializer->readLightMap(...)
mCustomSerializer->readColourMap(...)
mCustomSerializer->close()
A Typical write sequence:
mCustomSerializer->create(...)
mCustomSerializer->writeHeightMap(...)
mCustomSerializer->writeBlendMaps(...)
mCustomSerializer->writeLightMap(...)
mCustomSerializer->writeColourMap(...)
mCustomSerializer->close()
All those functions and their parameters are predefined virtual...
So, anyone can derive from your Base/Default Serializer class and override some of those steps, its up to them what to do in those steps, they can:
For Save:
- Write the data to seperate files if they desire so
- They can convert the data in whatever format before writing
- They can compress the data before writing
- They can IGNORE the data
For Load:
- They can read from different files if they desire so
- They can convert the data back
- They can uncompress the data
- They can return FALSE to state that they will not supply/use that part of the data or return a preset value
Thats what i am offering, complete freedom and it can be included in 1.7, all you need to do is create a DefaultSerializer class and plug it in by default which does the exact same thing your current code flow does...
Also the OPEN(...) function can verify a special chunk in the given file/stream to verify that the data file is compatible with the current serializer, or you can even go further and use the factory format, read the MAGIC from file and use the compatible serializer for reading etc...
This would make it sooo simple and most importantly make it POSSIBLE to read and write any terrain format you like seamlessly, only requirement is the serializer to respond to your abstract interface exactly as you want ...
Re: New Terrain Early Shots
Posted: Fri Feb 19, 2010 4:43 pm
by stealth977
Code: Select all
void Terrain::save(StreamSerialiser& stream)
{
// wait for any queued processes to finish
waitForDerivedProcesses();
if (mModified)
{
// When modifying, for efficiency we only increase the max deltas at each LOD,
// we never reduce them (since that would require re-examining more samples)
// Since we now save this data in the file though, we need to make sure we've
// calculated the optimal
Rect rect;
rect.top = 0; rect.bottom = mSize;
rect.left = 0; rect.right = mSize;
calculateHeightDeltas(rect);
finaliseHeightDeltas(rect, false);
}
BaseTerrainSerializer *mCustomTerrainSerializer = TerrainGlobalOptions::getDefaultSerializer();
mCustomTerrainSerializer->create(stream);
mCustomTerrainSerializer->writeHeader(mAlign, mSize, mWorldSize, mMaxBatchSize, mMinBatchSize, mPos);
mCustomTerrainSerializer->writeHeightMap(mHeightData);
mCustomTerrainSerializer->writeLayerDeclaration(mLayerDecl);
// Layers
checkLayers(false);
uint8 numLayers = (uint8)mLayers.size();
mCustomTerrainSerializer->writeLayerInstanceList(mLayers);
// Packed layer blend data
if(!mCpuBlendMapStorage.empty())
{
// load packed CPU data
int numBlendTex = getBlendTextureCount(numLayers);
for (int i = 0; i < numBlendTex; ++i)
{
PixelFormat fmt = getBlendTextureFormat(i, numLayers);
size_t channels = PixelUtil::getNumElemBytes(fmt);
size_t dataSz = channels * mLayerBlendMapSize * mLayerBlendMapSize;
uint8* pData = mCpuBlendMapStorage[i];
mCustomTerrainSerializer->writeBlendMap(pData);
}
}
else
{
if (mLayerBlendMapSize != mLayerBlendMapSizeActual)
{
LogManager::getSingleton().stream() <<
"WARNING: blend maps were requested at a size larger than was supported "
"on this hardware, which means the quality has been degraded";
}
uint8* tmpData = (uint8*)OGRE_MALLOC(mLayerBlendMapSizeActual * mLayerBlendMapSizeActual * 4, MEMCATEGORY_GENERAL);
uint8 texIndex = 0;
for (TexturePtrList::iterator i = mBlendTextureList.begin(); i != mBlendTextureList.end(); ++i, ++texIndex)
{
// Must blit back in CPU format!
PixelFormat cpuFormat = getBlendTextureFormat(texIndex, getLayerCount());
PixelBox dst(mLayerBlendMapSizeActual, mLayerBlendMapSizeActual, 1, cpuFormat, tmpData);
(*i)->getBuffer()->blitToMemory(dst);
size_t dataSz = PixelUtil::getNumElemBytes((*i)->getFormat()) *
mLayerBlendMapSizeActual * mLayerBlendMapSizeActual;
mCustomTerrainSerializer->writeBlendMap(tmpData);
}
OGRE_FREE(tmpData, MEMCATEGORY_GENERAL);
}
if (mCpuTerrainNormalMap)
{
// save from CPU data if it's there, it means GPU data was never created
mCustomTerrainSerializer->writeNormalMap((uint8*)mCpuTerrainNormalMap->data);
}
else
{
uint8* tmpData = (uint8*)OGRE_MALLOC(mSize * mSize * 3, MEMCATEGORY_GENERAL);
PixelBox dst(mSize, mSize, 1, PF_BYTE_RGB, tmpData);
mTerrainNormalMap->getBuffer()->blitToMemory(dst);
smCustomTerrainSerializer->writeNormalMap(tmpData);
OGRE_FREE(tmpData, MEMCATEGORY_GENERAL);
}
ETC....ETC...ETC....
mCustomTerrainSerializer->close();
mModified = false;
}
Example SAVE Flow... ( this way user can use TerrainGlobalOptions::setDefaultSerializer(&MyCustomSerializer) )
Re: New Terrain Early Shots
Posted: Fri Feb 19, 2010 7:19 pm
by sinbad
As I say, the trouble with that multi-method approach is that it doesn't work with packed, streamed data which evolves over time. By defining a specific set of data steps, and the order in which they happen, you lock the packed data format into a specific read order which is just too restrictive - either that or you force support of random-access data which is also unacceptable. I've dealt with packed data like .mesh for years and one thing you never want to do is close off the option to change the format later.
Yes, you could implement the current sequence this way but it would lock in the order & granularity forever.
I say again, you want a single unified load / save. Or, more to the point, I won't accept this unless it does it that way. The format & sequence is then up to the serialiser itself which is vastly more flexible.
Re: New Terrain Early Shots
Posted: Mon Feb 22, 2010 6:38 pm
by sinbad
I'm afraid I'm having to make a late breaking change to the new terrain API, caused by a bug with the way statics are initialised.
TerrainGlobalOptions is not longer something you call statically. Instead, it's a Singleton which you have to construct before you use any other terrain classes, and destroy after you've finished with any terrain features.
I apologise for having to make this change so late, but some problems with cross-static initialisation made this setup untenable, I should have spotted it earlier. It's not that hard to adapt to, just one extra instance to manage, and change to TerrainGlobalOptions::getSingleton() to call methods (or call your local member variable of it). This change will fix the linux static build (there are still some random issues with the windows static build which are unresolved despite many hours of debugging).
Re: New Terrain Early Shots
Posted: Mon Feb 22, 2010 8:10 pm
by BTolputt
I get the feeling from your last comment on the bug report that you might be giving up on the Windows static build of Terrain... Is this the case or am I worrying prematurely here?
Re: New Terrain Early Shots
Posted: Mon Feb 22, 2010 8:42 pm
by CABAListic
It just suggests that he needs a break. When you've spent hours on a ridiculously stupid bug, eventually you'd not even see the solution if it appeared right in front of you

The fix will be found eventually, just perhaps not today.
Re: New Terrain Early Shots
Posted: Mon Feb 22, 2010 8:58 pm
by BTolputt
Oh, I understand the need for a break and wasn't suggesting he sacrifice sleep & sanity to fix it. Hell, open-source volunteer and all that, regardless of my desires he can say "stuff it". I just wanted clarity on whether it was going to be fixed or ignored going forward.
Perhaps I am projecting my experience from other projects (sadly, some commercially released!) where I hear something along the lines of "
Yeah, we know there is a bug when it is compiled/used that way. We recommend you just don't use it that way (ignoring the fact we say that you can in the manual/sales brochure)". Extrapolating to this case, I was afraid of something along the lines of "
Yes, we support statically linked OGRE... unless you are using the Terrain plugin, in which case you're stuffed". Bad Ben
My main worry now is that the underlying issue made evident in this Terrain bug might be hidden elsewhere in the static build. The description of the problem in the bug tracker implies that there is linker weirdness between static libraries, which could show up elsewhere given the right circumstances.
Re: New Terrain Early Shots
Posted: Mon Feb 22, 2010 9:52 pm
by jacmoe
BTolputt wrote:I get the feeling from your last comment on the bug report that you might be giving up on the Windows static build of Terrain... Is this the case or am I worrying prematurely here?
You are indeed worrying prematurely here.
Re: New Terrain Early Shots
Posted: Tue Feb 23, 2010 12:28 pm
by sinbad
I'll definitely come back to it, but I could use some help though. After almost an entire day of debugging this one issue and encountering utterly random behaviour, I'm burned out on it and have to get on with something else. I think I need more inspiration on what could be causing it because I've been through all the usual suspects as far as I'm aware - runtime library consistency, _HAS_ITERATOR_DEBUGGING and _SECURE_SCL variants, static initialisation ordering, memory corruption. How in hell can memory become corrupt between the constructor of a class, and the first line of a method that constructor calls (with no intervening C++ code), both of which are defined in the same compilation unit so it's impossible for the 2 bits of code have had different compile settings??? It defies all logic and I was about to start rocking in a corner unless I took a break.
I'd really appreciate some help on this, my brain is melting.
Re: New Terrain Early Shots
Posted: Tue Feb 23, 2010 1:01 pm
by CABAListic
I can offer a fresh look tonight, although your debug-fu is probably ahead of mine

But since the issue does not show on Linux, I could try a MinGW build for comparison, there is a minimal chance that might offer some new insight.
Re: New Terrain Early Shots
Posted: Tue Feb 23, 2010 3:47 pm
by stealth977
Sinbad:
Today I filed a bug report about certain functions modifying the terrain but not setting the MODIFIED Flag, so the changes are not saved to terrain data files...
Also, i was wondering if you will accept the patch i submitted with extra functions: setSize() / setWorldSize() etc? Because having a modified OGRE dependency currently slows down Ogitor development, if the patch will be rejected, i may try to use a different workaround...
Re: New Terrain Early Shots
Posted: Tue Feb 23, 2010 4:04 pm
by sinbad
Thanks for the bug report.
I'm deferring all enhancement patches until 1.7 final is done, that's my top priority. I took a quick look at the patch and it'll probably be fine barring cosmetic changes (assuming it works

)
Re: New Terrain Early Shots
Posted: Tue Feb 23, 2010 10:32 pm
by BTolputt
sinbad wrote:I'd really appreciate some help on this, my brain is melting.
I'm busy for the next two days (upcoming
day-job software release), but I'll take a look into it first thing Friday if it hasn't been solved by then. I'm not an expert (or even an intermediate) OGRE developer but I am an experienced C/C++ developer and my take is that this is related to how the application is compiled/linked rather than something about OGRE internals... At worst, I'll waste my time in parallel to more qualified people finding the solution

Re: New Terrain Early Shots
Posted: Wed Feb 24, 2010 12:11 pm
by sinbad
Thanks to BTolputt this is now fixed. The problem was down to not resetting the "#pragma pack" used on on structures in the Grass and BezierPatch demos - completely unrelated to the terrain system of course but the unclosed struct packing must have leaked into the terrain struct definitions in the samples because of the way the static build includes all the sample headers in one file. This was causing memory layout mismatches.
BTolputt is now officially my hero.

This has to be one of the weirdest problems I've encountered and I'm not sure I would have spotted it without his input. I know now to add 'pragma pack' to my list of evil things to check when crazy things happen; it's been ages since I've been bitten my that and it didn't occur to me, especially as it was nowhere near the code I was looking at.
Re: New Terrain Early Shots
Posted: Wed Feb 24, 2010 12:31 pm
by Jabberwocky
Wow. Insano bug. Especially so because of how it leaked through from unrelated demo code.
Big fist bump to BTolputt.

Go open source!
Re: New Terrain Early Shots
Posted: Wed Feb 24, 2010 12:35 pm
by BTolputt
Honestly, it wasn't a "genius" moment for me, just a quick process of elimination.
- Variable pointers were off in the code run in the sample but not the library, indicating something wrong with the definition of struct/class definitions in the header file.
- The member variable offsets only started going awry after a bool (single byte variable), indicating a packing problem
- So I did a global search for "#pragma pack" and checked each instance for proper "closure" (i.e. resetting to previous/default packing)
With that said, and now fixed (thanks for prompt attention on that
sinbad!), is it a known issue that the "blend" editing in Sample_Terrain works using the DirectX back-end but locks the CPU when using the OpenGL one?
Re: New Terrain Early Shots
Posted: Wed Feb 24, 2010 1:27 pm
by sinbad
BTolputt wrote:is it a known issue that the "blend" editing in Sample_Terrain works using the DirectX back-end but locks the CPU when using the OpenGL one?
Hmm, I hadn't noticed that, must be something to do with the blitting. I'll log a bug and investigate.
Re: New Terrain Early Shots
Posted: Wed Feb 24, 2010 9:26 pm
by BTolputt
sinbad wrote:Hmm, I hadn't noticed that...
Great, I'll get a reputation now for the guy who holds up a release due to his "esoteric uses of OGRE"... My experience in the development career is that this is not a good thing!

Re: New Terrain Early Shots
Posted: Thu Feb 25, 2010 9:48 am
by Jabberwocky
BTolputt wrote:the guy who holds up a release...
Fist bump revoked!
