Sign in to follow this  
NSDuo

can't allocate structures

Recommended Posts

First thing I want to mention is I'm working on a MacBook Pro with OS X 5.6 and I'm using C plusplus in Xcode 3. I'm having some trouble with my OBJ loader. I created a bunch of structures within structures to organize the data as well as allowing me to create other model loaders. However my program keeps crashing. Here is my code: data.h:
#include "global.h"

typedef struct vertex
{
	// vertex location
	float x, y, z;
	
	// uv co-ordinates
	float u, v;
	
	// vertex normals
	float nx, ny, nz;
	
	// what bone is the vertex attached to?
	// temp space
	
	// level of influene from bone to vertex
	float inf;
	
} vert;

typedef struct face
{
	// each face can have a maximum of 4 verticies
	vert *points;
	
} face;

typedef struct object
{

	int noOfVerts;
	int noOfFaces;
	face *faces;
	
} obj;

obj *loadObj(std::string file);

obj.cpp:
#include "data.h"

obj *loadObj(std::string file)
{
	
	int i;
	FILE *filein;
	int totalverts = 0, totalfaces = 0;
	vert *tempVerts = NULL;
	obj *tempStorage;
		
	char oneline[255];
	
	filein = fopen(file.c_str(), "rt");
	
	while(!feof(filein))
	{
		fgets(oneline, sizeof(oneline), filein);
		
		if (oneline[0] == 'v' && oneline[1] == ' ')
		{
			totalverts ++;
			
		} else if (oneline[0] == 'f' && oneline[1] == ' ')
		{
			totalfaces ++;
			
		}
	}
	
	
	tempVerts = new vert [totalverts];
	if (tempVerts == NULL)
	{
		printf("error");
	}
	
	tempStorage->faces = new face [totalfaces];
	if (tempStorage->faces == NULL)
	{
		printf("error");
	}
	
	
	rewind(filein);

	int tempint = 0, spamint = 0, uvcount = 0;
	int currentFace = 0;
	int temparray[4];
	
	while (!feof(filein))
	{
		fgets(oneline, sizeof(oneline), filein);
		
		if (oneline[0] == 'v' && oneline[1] == ' ')
		{
			fscanf(filein, "%f", &tempVerts[tempint].x);
			fscanf(filein, "%f", &tempVerts[tempint].y);
			fscanf(filein, "%f", &tempVerts[tempint].z);
			
			tempint ++;
		} else if (oneline[0] == 'v' && oneline[1] == 't')
		{
			fscanf(filein, "%f", &tempVerts[uvcount].u);
			fscanf(filein, "%f", &tempVerts[uvcount].v);
			
			uvcount ++;
		} else if (oneline[0] == 'v' && oneline[1] == 'n')
		{
			fscanf(filein, "%f", &tempVerts[spamint].nx);
			fscanf(filein, "%f", &tempVerts[spamint].ny);
			fscanf(filein, "%f", &tempVerts[spamint].nz);
			
			spamint ++;
		} else if (oneline[0] == 'f' && oneline[1] == ' ')
		{
			
			for (i=0;i>4;i++)
			{
				
				tempStorage->faces[currentFace].points = new vert [4];
				
				fscanf(filein, "%i", temparray[i]);
				
				tempStorage->faces[currentFace].points[i].x = tempVerts[temparray[i]].x;
				tempStorage->faces[currentFace].points[i].y = tempVerts[temparray[i]].y;
				tempStorage->faces[currentFace].points[i].z = tempVerts[temparray[i]].z;
				
				currentFace ++;
			}
		}
	}
	
	fclose(filein);
	
	delete[] tempVerts;
	
	return(tempStorage);
	
}

So what happens is right after I declare tempStorage near the top of obj.cpp, one of the structures inside isn't given a memory address. the value of tempStorage is 0x970ac367 (seems like a valid address) the value of faces is 0x55c3c95e (again a valid address) the value of points is nothing, everything inside points is nothing too. According to another thread I started on iDevGames, my program is crashing because I'm accessing variables with no memory address, and without a memory address they don't belong to my program. So why is this happening and how to I fix it?

Share this post


Link to post
Share on other sites
obj *tempStorage;

is just a pointer, a dangling pointer that pointers to garbage (not even null) unless i over looked something you havent allocated anything to that pointer.

you need

obj *tempStorage = new obj;

before you can do

tempStorage->

the following will crash, tempStorage is pointing to garbage it looks like

tempStorage->faces = new face [totalfaces];

Edit:

When i say garbage i mean its hasnt been initialized by you, either to null or memory allocated by you. So it gets random values, which may look valid but are not.

Also, youre using c++, pass your strings by constant reference "const string&"
if youre not sure why, look up parameter passing in c++

typedef struct face {

} face;

is pointless in c++, more so in this case, your typedef'ing face as face?
where with

typedef struct object {

} obj;

you are shortening the type name object by 3 letters, if you want alternate names i suggest simply

struct object {

};

typedef object obj;

which at first glance looks more like c++, the whole thing threw me off there for a second, i thought you were using c, till i seen "string" being used

Share this post


Link to post
Share on other sites
The thing is, you are barely using C++. This is a lot easier if you make full use of the C++ language - in particular the extensive standard library provided for you.

First off, some mistakes.

In C++, you don't need to use the typedef struct idiom.

typedef struct vertex
{
// ...
} vertex;


Just use:

struct vertex
{
// ...
};


Here:

tempVerts = new vert [totalverts];
if (tempVerts == NULL)
{
printf("error");
}


In this snippet, there are two problems. First off, vanilla "new" can never return NULL. It will throw an exception of type std::bad_alloc if it cannot give you a valid pointer. Secondly, even if it *did* return NULL, simply printing "error" isn't really a great response. You need some level of error handling, which means informing the caller of the error condition (via an exception or return code). You may also want error reporting, so that the user of the program knows what went wrong.

Next up:

filein = fopen(file.c_str(), "rt");


Technically, this error was before the other one, but whatever. In the place most likely to be the source of an error, you didn't check for one! fopen() returns NULL if the file cannot be opened. Your program will crash (at best) in this case! This is somewhere you really should check for errors.

A loop like the below won't execute. Why? Well, "i" starts at 0. 0 isn't bigger than 4, so the loop body is skipped. Also, I would use more whitespace around the expressions, it makes it easier to read.

for (i=0;i>4;i++)
{
// ...
}



Finally:

Foo *foo;


Not an exact quote, but it stands for the various areas you were using pointers in your code. In idiomatic C++, it is fairly rare to need to use raw pointers. Usually, we wrap such areas of code in an object that can manage the memory correctly for us.

Anyway, this is how I would do it. Count the number of pointers [grin]

// data.h

#include <vector>
#include <string>

struct vertex
{
// vertex location
float x, y, z;

// uv co-ordinates
float u, v;

// vertex normals
float nx, ny, nz;

// what bone is the vertex attached to?
// temp space

// level of influence from bone to vertex
// float inf;
};

struct face
{
// each face can have a maximum of 4 verticies
vertex points[4];
};

struct object
{
std::vector<face> faces;
};

// Pass complex types like std::string via const reference
object loadObj(const std::string &file);

// data.cpp

#include "data.h"
#include <fstream>
#include <sstream>
#include <cassert>
#include <stdexcept>

namespace
{
// helper
// ensures that the index is always valid
vertex &get(std::vector<vertex> &v, int index)
{
assert(index >= 0);
if( index >= v.size() )
{
v.resize(index);
}
return v[index];
}
}

object loadObj(const std::string &filename)
{
std::ifstream file(filename.c_str());

if(!file)
{
throw std::runtime_error("failed to open file: " + filename);
}

std::vector<vertex> vertices;
object obj;

int currentVertex = 0, currentUV = 0, currentNormal = 0;

std::string line;
while(std::getline(file,line))
{
if(line.size() < 2)
{
// error checking/reporting
throw std::runtime_error("file corrupt: <line number> <other info>");
}
else if(line[0] == 'v' && line[1] == ' ')
{
std::stringstream stream(line.substr(2));

float x, y, z;

if(stream >> x >> y >> z)
{
vertex &v = get(vertices,currentVertex);
v.x = x;
v.y = y;
v.z = z;

++currentVertex;
}
else
{
// error checking/reporting
throw std::runtime_error("file corrupt: <line number> <other info>");
}
}
else if (line[0] == 'v' && line[1] == 't')
{
std::stringstream stream(line.substr(2));

float u, v;

if(stream >> u >> v)
{
vertex &vert = get(vertices,currentUV);
vert.u = u;
vert.v = v;

++currentUV;
}
else
{
// error checking/reporting
throw std::runtime_error("file corrupt: <line number> <other info>");
}
}
else if (line[0] == 'v' && line[1] == 'n')
{
std::stringstream stream(line.substr(2));

float nx, ny, nz;

if(stream >> nx >> ny >> nz)
{
vertex &v = get(vertices,currentNormal);
v.nx = nx;
v.ny = ny;
v.nz = nz;

++currentNormal;
}
else
{
// error checking/reporting
throw std::runtime_error("file corrupt: <line number> <other info>");
}
}
else if (line[0] == 'f' && line[1] == ' ')
{
face f;
for(int i = 0 ; i < 4 ; ++i)
{
int index;

if(file >> index)
{
f.points[i] = vertices.at(index);
}
else
{
// error checking/reporting
throw std::runtime_error("file corrupt: <line number> <other info>");
}
}
obj.faces.push_back(f);
}
else
{
// error checking/reporting
throw std::runtime_error("file corrupt: <line number> <other info>");
}
}

return obj;
}


This code uses some types you may not be familiar with. std::vector is a dynamic array. The usage here is not typical, because we are reading the indices from an untrustworthy file. I went with the logic you were using for the file format. It might be easier if you broke the "vertex" struct into the position, texture co-ords and normals as separate types.

The exception types, std::runtime_error, can be used to inform the caller of errors:

int main()
{
try
{
object obj = loadObj("some.file.format");
}
catch(const std::exception &e)
{
// something went wrong.
}
}



The std::ifstream is an "input file stream". Its use is similar to std::cin.

Then we have std::stringstream. This type is another stream, but it reads and writes from a string. This means that we can load each line from the file, throw the line into a stringstream and then begin to extract from the stringstream into actual types. The reason for doing this is to simplify the logic in the case of a type error.

Finally: a warning. While I did check the code compiled, it is not tested. It probably contains some bugs due to my misunderstanding of the file format or your previous code.

Share this post


Link to post
Share on other sites
The garbage pointer makes sense, but unfortunately the obj *tempStorage = new obj; thing didn't work and the inner structures are still empty. I also tried obj *tempStorage = new obj[1];, but that didn't work either.

Share this post


Link to post
Share on other sites
Are you loading a custom format? I use my own custom export script for Blender and it writes how many vertices there are before the actual vertex data. I recommend this method rather than wasting countless cycles counting your vertices. After reading the number of vertices I allocate my vertices. I then read them all from the file and move onto my materials section. All of the sections of my file are like this. I even go so far as to separate my triangles from quads so I don't have to call glBegin() a bajillion times.

I tried using ifstream to load my binary files and ifstream would cause my program to crash after so many reads. If you ever use binary files, I don't recommend ifstream. Also, ifstream doesn't afford the freedom that fscanf() does.

[Edited by - kittycat768 on January 16, 2009 8:39:56 PM]

Share this post


Link to post
Share on other sites
Quote:
I tried using ifstream to load my binary files and ifstream would cause my program to crash after so many reads. If you ever use binary files, I don't recommend ifstream. Also, ifstream doesn't afford the freedom that fscanf() does.
@The OP: I would take the above advice with a grain of salt. Although it's not unheard of for there to be bugs in widely-used implementations of the standard C++ library, such claims should in general be viewed with a healthy does of skepticism (statistically speaking, the actual cause of the problem is more likely to be user error).

@kittycat: Can you clarify in what way C IO functions offer more freedom than the SC++L 'stream' classes?

Share this post


Link to post
Share on other sites
Quote:
Original post by jyk
Quote:
I tried using ifstream to load my binary files and ifstream would cause my program to crash after so many reads. If you ever use binary files, I don't recommend ifstream. Also, ifstream doesn't afford the freedom that fscanf() does.
@The OP: I would take the above advice with a grain of salt. Although it's not unheard of for there to be bugs in widely-used implementations of the standard C++ library, such claims should in general be viewed with a healthy does of skepticism (statistically speaking, the actual cause of the problem is more likely to be user error).

@kittycat: Can you clarify in what way C IO functions offer more freedom than the SC++L 'stream' classes?


You gotta be a little lenient with me. I just discovered an hour ago that cout is flexible... I now know that:

int bob = 32;
cout << "Bob is age: " << bob << ".\n"






will yield: "Bob is age: 32." (new line)

I actually have opened up a thread about why ifstream is spitting garbage at me.

EDIT: I botched my export script.. lol /apologizes to ifstream. I'm trying really hard to learn the standard C++ classes and templates so I stop looking like a complete idiot on these forums... [rolleyes]

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this