in the examples I have some problem with using Region directly since there is something else called Region somewhere.
fixed that.

implemented flush and prefetch and fixed a bug inside the eraseBlock function that happens when using RLE and paging.
bug happened in the following scenario:

load a block and unpack it
it's now in the rle-block-cache.
unload the block (it's still in the rle-cache, which now contains an invalid pointer)
load a new block (might get placed into the slot where the invalid block is)
causes invalid block to be packed
-> unpredictable behavior.

eraseBlock now looks for the block in the rle-cache (it's obviously in there since we just accessed it for saving), and removes it.
please look into my method of removing a single block.
I'm not sure if this is a good method, but I believe it to be safe and fast.

I updated the PagingExample to test and show off the flush/prefetch functions

Ok, I've applied the patch. A couple of small points though - maybe just 'prefetch' and 'flush' is enough rather than 'prefetchRegion' and 'flushRegion'. Also I always use brackets even for one line for loops or if statements, and I place them on their own line. I guess you like your code to look compact, whereas I tend to spread mine out

When prefetching:

• When the whole region cannot be prefetched maybe it's best to just prefetch as much as possible? I doubt if this will happen much anyway - I imagine the prefetching is used on fairly small regions.
• I guess you just see the TODO as an optimisation, right? Because the blocks will get unloaded anyway as new ones are loaded.

When flushing:

• Is it useful to return the number of blocks flushed? It's kind of an implementation detail and I'm not sure what the user would do with this information. And what is the purpose of 'm_uBlockSideLengthPower*3'?

Actually, I would argue that it's normally not in there (unless I misunderstand). Leaving aside the prefetch/flush, the usual reason why eraseBlock is called is because the block is so old that it is being removed. If it's old then it should have been taken out the uncompressed cache long ago. Maybe we need more than one eraseBlock function here.

As for my 'FIXME's they weren't aimed at you (although of course you are welcome to fix them). They were just notes for the future for things that I hadn't properly thought about yet. In the case of 'setMaxNumberOfBlocksInMemory' I think a complete flush would be easier and safer (as you say, it will probably only be called at the start). I feel we are starting to develop some complexity in this class now, so I'd rather keep everything easy and safe rather than risk bugs.

actually that is what happens. since I'm shifting to block coordinates, I end up getting more voxels than anticipated if someone enters not exact block begin and end coordinates for the region.


yes, I did not implement it for the same reason you posted later... keeping it simple.


I didn't say number of blocks, I said number of voxels.
shifting by m_uBlockSideLengthPower*3 is the same as multiplying by the number of voxels in a block.
I'm not sure what one would do with it except to test for != 0, so we could also return a boolean if you prefer.




nope it's in there. we just tried to save it, for saving the user *may* access it, increasing the timestamp and making sure it's back in the uncompressed cache.

so the ideal fix is:
1. give the block directly to ConstVolumeProxy, removing all calls to getUncompressed Block
2. uncompressing it manually in ConstVolumeProxy if necessary (without adding to the uncompressed cache)
3. forcing the uncompressed cache to be smaller than the paging cache at all times.
4. creating a second erase function just for the flush function.


okay lets do that. and warn the user in the docs that this function should only be called at the beginning.


sure, there isn't anything else that's supposed to be flushed/prefetched anyway

jop, if you want my additions to polyvox to look your way I can do that from now on.
it's probably better if it's consistent

Actually I'm referring to this line:



In which case it will not prefetch anything. Maybe just take that out so it can prefectch as much as it can?


Ok, the 'm_uBlockSideLengthPower*3' makes sense now. But actually I'm not sure if even a boolean is useful - can the function actually fail? Maybe it should just return void.



I agree that this is true in the case that the user is saving it, I was just pointing out that the eraseBlock() function gets called even when the streaming is disabled. In this case the block will probably be very old and not in the cache. But I think we are agreeing here, because of your point (4) below.



For 1 and 2 I'm not sure of the exact logic... maybe the getUncompressedBlock() function should take an iterator to the block? If the iterator is invalid (the usual case) then it get's looked up as usual. If it is valid (because we came through the ConstVolumeProxy) then we can skip the map lookup. I'm not sure the ConstVolumeProxy has to handle the compression itself though. Anyway, it's really an optimisation so don't feel you have to address it.

For 3 I guess so... maybe assert in debug mode and clamp the values in release?

For 4, yes. Or otherwise I guess the eraseBlock() function could take a boolean parameter.


Yep, agreeed.


Sure, I did notice you kept the variable naming in line with the existing scheme. There are some places where I'm inconsistant anyway but in general I try to avoid it.

oh... good point. I'll change it to void and just prefetch as much as possible.


well.. I just wanted to let the user somehow know if anything actually happened... and maybe even "how much" happened.
but it's not really necessary.
if you want it out of there, I'll remove it.


if paging is disabled we should make sure eraseBlock is never called, since it's just unnecessary overhead (and only happens on a call to the volumes destructor, not because the block is too old)


won't work... if getUncompressedBlock is handling the uncompression, this block is back in the uncompressed cache and will cause the same troubles I just fixed by changing eraseBlock.


sure, but we need to change both the setMaxBlocksInMemory and setMaxUncompressedBlocks functions.

depends on how we fix 1&2...

ok, implemented those changes and created a flushAll function that is called inside setMaxBlocksInMemory and ~Volume
it simply clears out the m_pBlocks and calls the unload function

Ok, I think I'm with you now



In your patch I didn't see anything which addresses this, but I think that's fine. We can leave these points until a later date as it would be good to merge this into the trunk soon.



Great, I've applied the patch with minor formatting changes but no logic changes. Is there anything else you want to add before we merge into the trunk? I will look over the code again as I update the class documentation, but basically I'm happy with it

no more changes from my side.
it works perfectly in my application this way.

jup only some more descriptive documentation and everything should be good

Great, I think we can get this in over the next few days then. The only documentation we really need is the class documentation, and I'm happy to write that as I also need to review the stuff about volume compression.

If you want to write some documentation as well then maybe a tutorial based on your paging example would be useful? Some thing like we already have for the basic example: http://www.thermite3d.org/resources/documentation/polyvox/documentation/tutorial1.html.

Only if you get a chance of course, and we can still merge without it so it can also come later.

Unfortunatly I didn't get as much done on the documentation as I had hoped... it takes a lot of time! But I did make a start and will aim to do some more over the week.