patterncppMinor
Virtual Texturing - Page Indirection Table
Viewed 0 times
pagetexturingindirectionvirtualtable
Problem
I'm working on a Virtual Texturing library for mobile devices based on OpenGL-ES.
This is the
vt_page_indirection_table.hpp:
```
#ifndef VTLIB_VT_PAGE_INDIRECTION_TABLE_HPP
#define VTLIB_VT_PAGE_INDIRECTION_TABLE_HPP
namespace vt
{
// ======================================================
// PageIndirectionTable:
// ======================================================
class PageIndirectionTable
{
public:
// Default constructor initializes the table texture.
// Might throw and exception if initialization fails.
PageIndirectionTable(const int vtPagesX, const int vtPagesY, int vtNumLevels);
// Frees the OpenGL texture handle.
virtual ~PageIndirectionTable();
// Bind the texture as current OpenGL state.
void bind(int texUnit = 0) const;
// Draws the indirection texture as a screen-space quadrilateral for debug visualization.
// Manually binding the texture is not necessary. 'overlayScale' controls the scale of the overlay quad. From 0 to 1.
void visualizeIndirectionTexture(const float overlayScale[2]) const;
// Update the indirection table texture. This is called whenever the page cache changes.
// Array size must be 'PageTable::TotalTablePages'. Must bind first with PageIndirectionTable::bind().
virtual void updateIndirectionTexture(const struct CacheEntry * const pages) = 0;
// Write every mip-level of the indirection table to image files.
// The file name/path should not include an extension. Each level will be named ( pathname + "_level_number" ).
virtual bool writeIndirectionTextureToFile(const std::string & pathname, bool recolor) const = 0;
protected:
// OpenGL texture handle:
GLuint indirectionTextureId;
// Num mip-levels in the Virtual Texture and per-level page counts:
This is the
PageIndirectionTable, one of the library components I would like to have some feedback on:vt_page_indirection_table.hpp:
```
#ifndef VTLIB_VT_PAGE_INDIRECTION_TABLE_HPP
#define VTLIB_VT_PAGE_INDIRECTION_TABLE_HPP
namespace vt
{
// ======================================================
// PageIndirectionTable:
// ======================================================
class PageIndirectionTable
{
public:
// Default constructor initializes the table texture.
// Might throw and exception if initialization fails.
PageIndirectionTable(const int vtPagesX, const int vtPagesY, int vtNumLevels);
// Frees the OpenGL texture handle.
virtual ~PageIndirectionTable();
// Bind the texture as current OpenGL state.
void bind(int texUnit = 0) const;
// Draws the indirection texture as a screen-space quadrilateral for debug visualization.
// Manually binding the texture is not necessary. 'overlayScale' controls the scale of the overlay quad. From 0 to 1.
void visualizeIndirectionTexture(const float overlayScale[2]) const;
// Update the indirection table texture. This is called whenever the page cache changes.
// Array size must be 'PageTable::TotalTablePages'. Must bind first with PageIndirectionTable::bind().
virtual void updateIndirectionTexture(const struct CacheEntry * const pages) = 0;
// Write every mip-level of the indirection table to image files.
// The file name/path should not include an extension. Each level will be named ( pathname + "_level_number" ).
virtual bool writeIndirectionTextureToFile(const std::string & pathname, bool recolor) const = 0;
protected:
// OpenGL texture handle:
GLuint indirectionTextureId;
// Num mip-levels in the Virtual Texture and per-level page counts:
Solution
Generally, the code looks very well-written. Good job!
Small improvements:
Nitpicky improvements:
Also, have you considered static polymorphism via the CRTP? I can't say whether this applies without seeing the client code. If it does apply, then you can gain some performance by dropping the vtable. However, the real world effects are probably negligible.
Small improvements:
- Use
std::uint8_tinstead ofuint8_t. The former is C++11 and the latter is C99. Same forstd::uint16_tand any other fixed-width integers.
- Drop
NonCopyableand explicitly delete the copy constructors.
- Drop the
structkeyword inconst struct CacheEntry * const pages. It's not needed in C++.
- $7.3.1.1/2 from the C++ Standard: The use of the static keyword is deprecated when declaring objects in a namespace scope; the unnamed-namespace provides a superior alternative. I'm looking at
static constexpr GLenum indirectionTexMinFilter = GL_NEAREST_MIPMAP_NEAREST;and similar definitions.
- Declare
numLevelsasconst. As far as I can see, you only write tonumLevelsin the constructor's member initializer list. DeclaringnumLevelsasconstwill avoid accidental writes and clarifies intent.
Nitpicky improvements:
- Get in the habit of writing
for (int i = 0; end != i; ++i)instead offor (int i = 0; i != end; ++i)whenendisconstor an r-value. This way, you will get a compiler error if you accidentially writeend = iin the loop (assignment instead of test). Same goes for=, and especially==. I'd even prefer that style just for `just to stay consistent. This goes for all your rawfor` loops.
Also, have you considered static polymorphism via the CRTP? I can't say whether this applies without seeing the client code. If it does apply, then you can gain some performance by dropping the vtable. However, the real world effects are probably negligible.
Context
StackExchange Code Review Q#71111, answer score: 4
Revisions (0)
No revisions yet.