Back to For Beginners

Mesh class question

For Beginners

Started by toro August 23, 2010 04:41 PM

7 comments, last by toro 13 years, 8 months ago

toro

122

Author

August 23, 2010 04:41 PM

After several changes in the way the mesh information is stored in my project, I need some advices on how to improve it. Right now, the code looks like this:

class vector{public:    float x, y, z;public:    inline vector(void) {};    inline vector(const float tx, const float ty, const float tz)    { x = tx; y = ty; z = tz;}    inline void copy(const vector &v)    { x = v.x; y = v.y; z = v.z;}    ...}

class vector_container{public:     std::vector<vector> content;public:     ...     inline void vector_container::add_vector(vector *p)     { content.push_back(*p); }     inline vector& vector_container::operator[] (unsigned int i)     { return content; }     ...}

The above definition allows two ways to add vectors to the "container":

vector_container *pv = new vector_container(4);      pv->add_vector(new vector(100.0f, 100.0f, 100.0f));      (*pv)[0].copy(vector(100.0f, 100.0f, 100.0f));

At first glance, for someone with more experience, my questions are:
1. Is ok to use std::vector to store mesh information like this?
2. The syntax for add_vector and operator[] is ok? or there is a better way to do it?

realkiran

100

August 23, 2010 05:17 PM

Quote:Original post by toro

At first glance, for someone with more experience, my questions are:
1. Is ok to use std::vector to store mesh information like this?
2. The syntax for add_vector and operator[] is ok? or there is a better way to do it?

1. yes
2. what is the point of vector_container? why not just use:

typedef std::vector<vector> vector_container;

also, you may want to name it something other than "vector" since that name is already used in the standard libraries.

dmatter

4,872

August 23, 2010 05:18 PM

Quote:Original post by toro
1. Is ok to use std::vector to store mesh information like this?

Using a std::vector is fine and the most appropriate of all standard library containers for this task.

Quote:2. The syntax for add_vector and operator[] is ok? or there is a better way to do it?

No, in fact add_vector is horribly broken. Look at your usage:

pv->add_vector(new vector(100.0f, 100.0f, 100.0f));

and then your implementation:

content.push_back(*p);

To invoke your function you allocate memory with new to make a vector object.
Inside your function you store a copy of that object in content.
Your function returns, the pointer to your allocated object is lost and the object is never deleted. [sad]

Change it to use a (const) reference, like this:

void vector_container::add_vector(const vector & v) { content.push_back(p); }

As for operator[], it's mostly fine; since the operator does not mutate any member variables you should define it with the const keyword to signal this to the compiler and the user:

vector& vector_container::operator[] (unsigned int i) const { return content; }

In closing, I'd like to point out that making variables public is usually considered bad (I'm looking more at vector_container here) and that the inline keyword, when used on member functions, is entirely ignored by compilers these days and so it's less commonly seen in modern code.

David Gill :: GitHub :: twitter .

toro

122

Author

August 23, 2010 08:30 PM

@realkiran: Your typedef solution would work, but I think the wrapper for std::vector is more comfortable for me. Your remark about "vector" name is also correct.

@dmatter: Fixed the add_vector() implementation and content became private. Thank you.

However there is a small issue with:

vector& vector_container::operator[] (unsigned int i) const { return content; }

if I put the const keyword there, the compiler will throw this error:

vector_container.h(37) : error C2440: 'return' : cannot convert from 'const vector' to 'vector &'

But this is not important, as the original implementation is ok. Thanks again.

dmatter

4,872

August 23, 2010 08:47 PM

Sorry, that's a brain-fart on my account.

Rather you ought to provide both const and non-const versions where the const version also returns a const reference:

vector & vector_container::operator[] (unsigned int i) { return content; }
const vector & vector_container::operator[] (unsigned int i) const { return content; }

std::vector has both of these for its operator[].

Without both a const-correct function such as the following would be impossible:

void outputX(const vector_container & vc, unsigned int i){    std::cout << vc.x;}

David Gill :: GitHub :: twitter .

Zakwayda

2,389

August 23, 2010 09:05 PM

Quote:Original post by realkiran
also, you may want to name it something other than "vector" since that name is already used in the standard libraries.

Using the name 'vector' is fine, provided you don't bring the two names into conflict via 'using' directives or declarations.

You can certainly use a different name if it's a concern, but there should be no problem with using the name 'vector' for your own class.

Quote:Original post by toro
@realkiran: Your typedef solution would work, but I think the wrapper for std::vector is more comfortable for me.

Unless you have a clear idea of what advantages using a wrapper offers, I think you should reconsider this.

Sometimes people will wrap another container type if they only want to expose a subset of its functionality (the standard library includes several such 'container adapters'). In general though, unless you have a good reason for doing so, creating a wrapper of this sort just makes your code less clear, and creates more work for you. Really, if all you're doing is storing some stuff in a vector, just use vector<stuff>.

Quote:Your remark about "vector" name is also correct.

You don't necessarily need to change the name (see above).

Quote:1. Is ok to use std::vector to store mesh information like this?

Generally speaking (and any technical problems with your code aside), it is totally ok to store mesh data in a vector.

Also, a couple of quick comments on your code:

class vector{public:    float x, y, z;public:    // You don't need the 'inlines' here (I think this was mentioned    // previously).        // Also, although there are a few folks who do advocate the (void)    // convention even in C++, most would recommend omitting the 'void'    // (i.e. just write vector()).    inline vector(void) {};        // Look up 'initializer lists', as that's what you'll want to use    // here. Also, you can just name the arguments x, y, and z if you    // want. With initializer lists there'll be no conflict in naming,    // and even with assignment as in your current code you can    // qualify the member variables with this-> to resolve the    // ambiguity.    inline vector(const float tx, const float ty, const float tz)    { x = tx; y = ty; z = tz;}        // The compiler will generate the equivalent of this automatically,    // so there's no need to define it explicitly.    inline void copy(const vector &v)    { x = v.x; y = v.y; z = v.z;}}

toro

122

Author

August 24, 2010 02:47 AM

@dmatter: It works. :)

@jyk: I've made all the changes you suggested for the vector class.
On the wrapper issue, basically I have two reasons for it (1) learning and (2) I want to impose a specific behavior for this class (memory allocation on construction). Unless is quite inefficient, I intend to keep it for a while, at least until I feel more comfortable with std::vector<vector>.

class vector_container{private:std::vector<vector> content;	public:vector_container::vector_container(unsigned int count) { content.resize(count); }vector_container::~vector_container() { content.clear(); }unsigned int vector_container::size() { return content.size(); }void vector_container::add_vector(const vector & v) { content.push_back(v); }vector& vector_container::operator[] (unsigned int i) { return content; }const vector& vector_container::operator[] (unsigned int i) const { return content; }};

BitMaster

8,652

August 24, 2010 03:28 AM

Quote:Original post by toro[...] (2) I want to impose a specific behavior for this class (memory allocation on construction).

You do know that std::vector already has a constructor which does exactly what you do?

Edit: On a sidenote:
- Doing a resize() and then using push_back() probably does not what you think it does in regards to memory allocation on construction.
- The destructor of vector already frees all its memory, calling clear() in your own destructor is not needed. Since you appear not to want a virtual destructor you could as well remove the now empty destructor.
- std::vector::size returns a variable of type std::vector::size_type (usually std::size_t) which is not necessary identical with unsigned int. At the very least, you should do a static_cast there to make the code warning free (striving for warning-free code is a much better learning experience than writing the wrapper in the first place).

Performance wise, your wrapper loses everything that comes out of the box with a normal C++SL container, including a fast swap and iterators (whose use over access by index can sometimes boost performance) and can be much more convenient in combination with for example BOOST_FOREACH.

[Edited by - BitMaster on August 24, 2010 4:28:21 AM]

toro

122

Author

August 24, 2010 07:54 AM

In the end, I replaced the "container" class with std::vector<vector3f>. There was no performance penalty versus the original version, but still - the wrappers were really not necessary.
Thanks for your suggestions. My code was "fixed" :)

Mesh class question

This topic is closed to new replies.

Popular Topics

Recommended Tutorials

Mesh class question

This topic is closed to new replies.

Popular Topics

Recommended Tutorials

Reticulating splines