5
\$\begingroup\$

I have written a small container class which groups a 3D position, a normal vector and a texture coordinate into one object. It uses the glm library for the actual data types (vec2 and vec3). This class is named Vertex. Here is the source:

Vertex.h

#ifndef CPP3D_VERTEX_H
#define CPP3D_VERTEX_H

#include <iostream>
#include <glm/glm.hpp>

namespace cpp3d {

class Vertex {

public:
    glm::vec3 position;
    glm::vec3 normal;
    glm::vec2 texCoord;
    Vertex();
    Vertex(const glm::vec3& position);
    Vertex(const glm::vec3& position, const glm::vec3& normal);
    Vertex(const glm::vec3& position, const glm::vec3& normal, const glm::vec2& texCoord);
    bool operator==(const Vertex& other);
};

std::ostream& operator<<(std::ostream &stream, const Vertex& vertex);
std::istream& operator>>(std::istream &stream, Vertex& vertex);

}

#endif

Vertex.cc

#include "Vertex.h"

namespace cpp3d {

using namespace glm;
using namespace std;

Vertex::Vertex() {
}

Vertex::Vertex(const vec3& position) :
        position(position) {
}

Vertex::Vertex(const vec3& position, const vec3& normal) :
        position(position), normal(normal) {
}

Vertex::Vertex(const vec3& position, const vec3& normal, const vec2& texCoord) :
        position(position), normal(normal), texCoord(texCoord) {
}

bool Vertex::operator ==(const Vertex& other) {
    return position == other.position && normal == other.normal && texCoord == other.texCoord;
}

ostream& operator<<(ostream &stream, const Vertex& vertex) {
    stream.write(reinterpret_cast<const char *>(&vertex), sizeof(vertex));
    return stream;
}

istream& operator>>(istream &stream, Vertex& vertex) {
    stream.read(reinterpret_cast<char *>(&vertex), sizeof(vertex));
    return stream;
}

}

As you can see I've overloaded the stream operators to write and read the data the object contains. For this I simply dump the whole object which results in writing eight float numbers so the output will be 32 bytes. I know that this is a very simple file format but I want to read the data as fast as possible so I don't want to parse text files. I also don't really care about different endiannesses (unless it can be implemented without speed impact). And I know the file format will change when I add more members to the class (This will most likely not happen but when it happens then I also want to change the file format anyway)

My main concern is if this code guarantees that it always writes and reads 32 bytes. I'm pretty new to C++ and my ancient C knowledge tells me that there is (or was) some kind of packed structs and aligned structs where the compiler could decide to align 8 bit values to 16 bits for some reason. I don't want some compiler (current or future ones) to store the 32 bit floats in 64 bit values because this obviously changes the binary data format. Can this happen with my code when compiled on specific platforms? If yes, what's the best way to write binary floats to a stream then? Shall I write each coordinate in each vector with a separate call to write()? Or are there other/better techniques to write and read the binary data?

If you see other issues with my code which are not related to my question please feel free to comment anyway. As a C++ newbie I'm eager to learn.

\$\endgroup\$
  • \$\begingroup\$ You should also indent the entire contents of the namespaces. \$\endgroup\$ – Jamal Jul 1 '14 at 16:13
  • 1
    \$\begingroup\$ As long as the implementations of the glm::vec* structs do not change, you can probably get away with this. It would be safer to write to each data member separately. Just make sure your class is trivially copyable. \$\endgroup\$ – jliv902 Jul 1 '14 at 18:24
6
\$\begingroup\$
  • Both std::istream and std::ostream are actually not defined in <iostream>, despite the fact that they look similar. They're respectively defined in <istream> and <ostream>.

    You should instead include these in the implementation file and include <iosfwd> (the iostream declaration forwarding library) in the header.

  • These data members should be private:

    glm::vec3 position;
    glm::vec3 normal;
    glm::vec2 texCoord;
    

    Otherwise, they will be accessible outside of the class. If they are not meant to be this way, then you should use a struct instead, which are public by default.

  • If you don't need a default constructor, then leave it out. The compiler will provide one for you.

  • Regarding operator==:

    Conditional operators overloads, such as operator==, should be const as they should not modify any data members:

    bool operator==(const Vertex& other) const;
    

    Its definition could also be split into multiple lines, which is easier to read than one long line:

    bool Vertex::operator==(const Vertex& other) {
        return position == other.position
            && normal == other.normal
            && texCoord == other.texCoord;
    }
    
\$\endgroup\$
3
\$\begingroup\$

static_assert:

You've tagged the question as C++11, so you can use static_assert to make sure you are not surprised if the sizes of the GLM types change in a future update of the library.

static_assert(sizeof(glm::vec3) == (sizeof(float) * 3), "These sizes must match!");

And also add a check at the end of your class to be sure that it is not getting unwanted padding:

static_assert(sizeof(Vertex) == (sizeof(float) * 8), "Wrong size for Vertex!");

(These checks can be placed in the global/namespace scope).

Definitely make the data members private:

Another reason to make the data members private is this: If you ever wish to change the vertex data types from a float vec3 to a vector of half-floats or even bytes, for space saving reasons (this is very common for console and mobile games) you won't have to change any use of the Vertex type in client code. Wrap all data access into get/set methods, so that if you ever need to change the underlaying data type, no external code will need change.

glm::vec3 Vertex::getPosition() const
{
    return glm::vec3(unpackHalfFloats(hfPosition));
}
// Example if your Vertex where to store data internally as compressed half-floats.

Write data members to file individually:

It might be better to write, and consequentially read, the data sub-member of the vectors individually. E.g.:

stream.write(reinterpret_cast<const char *>(&vertex.position.x), sizeof(float));
stream.write(reinterpret_cast<const char *>(&vertex.position.y), sizeof(float));
stream.write(reinterpret_cast<const char *>(&vertex.position.z), sizeof(float));

This will dodge any padding/packing problems. The only overhead here is the extra function calls, since the stream object is certainly buffered.

Floating-point equal comparison is "fuzzy":

Two floating point numbers are only equal if they are an exact binary copy of each other, so your operator == is likely to return false for very close numbers that your application would otherwise deem as as equal. E.g.: 0.0000001 and 0.00000015, for most purposes are the same number, but the == operator would judge them as different numbers. See this guide for more on the subject.

You might consider using a comparison with a bias, such as the example:

bool nearlyEqual(float a, float b, float epsilon)

provided on the previous link, and rewrite operator == to:

bool Vertex::operator == (const Vertex& other) const {
    return (nearlyEqual(position, other.position, FloatEpsilon)
         && nearlyEqual(normal,   other.normal,   FloatEpsilon)
         && nearlyEqual(texCoord, other.texCoord, FloatEpsilon));
}

Using a comparison epsilon suitable for your application.

\$\endgroup\$
  • \$\begingroup\$ I would avoid getters/setters if possible. Otherwise, getters can return a const& to prevent modifications. \$\endgroup\$ – Jamal Jul 6 '14 at 19:44
  • \$\begingroup\$ Yes, a const ref would be better for the current code. But in the scenario I've used to exemplify, converting from one data type to another would require a temp, which cannot be returned by ref. \$\endgroup\$ – glampert Jul 6 '14 at 19:47
2
\$\begingroup\$

A faster I/O method is to have your overloaded operators write into a buffer of {formatted} characters. After the buffer is filled, use std::ostream::write to write the entire buffer in one operation. This should reduce a lot of the overhead from each class writing to the I/O streams.

Similarly, read from the stream into a buffer. Each class should have methods to load their data members from the buffer (hint: buffer pointer that gets incremented after each data member is loaded).

In general, you will gain two areas of performance: reduction of calls to output functions and keeping the I/O devices busy. I/O devices like to operate on large chunks of data rather than many small pieces of data.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.