HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Virtual Texturing - Page Indirection Table

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
pagetexturingindirectionvirtualtable

Problem

I'm working on a Virtual Texturing library for mobile devices based on OpenGL-ES.

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:

  • Use std::uint8_t instead of uint8_t. The former is C++11 and the latter is C99. Same for std::uint16_t and any other fixed-width integers.



  • Drop NonCopyable and explicitly delete the copy constructors.



  • Drop the struct keyword in const 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 numLevels as const. As far as I can see, you only write to numLevels in the constructor's member initializer list. Declaring numLevels as const will avoid accidental writes and clarifies intent.



Nitpicky improvements:

  • Get in the habit of writing for (int i = 0; end != i; ++i) instead of for (int i = 0; i != end; ++i) when end is const or an r-value. This way, you will get a compiler error if you accidentially write end = i in 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 raw for` 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.