Sign in to follow this  
p0is0n

fread and std::istream::read

Recommended Posts

p0is0n    100
Hi all, I'm writing some file loading code from a tutorial. The tutorial uses fread, and my code is using std::istream::read. Reading the exact same file when I use my way :
	//Read in the vertices
	Vector3 *verts = new Vector3[object->numVerts];
	memset(verts, 0, sizeof(Vector3) * object->numVerts);
	m_file.read(reinterpret_cast<char*>(verts), previousChunk->length - previousChunk->bytesRead);
	previousChunk->bytesRead += m_file.gcount();


The bytes read before the code is 8, after the call they are 98. This is the code in the tutorial:
	// Allocate the memory for the verts and initialize the structure
	pObject->pVerts = new CVector3 [pObject->numOfVerts];
	memset(pObject->pVerts, 0, sizeof(CVector3) * pObject->numOfVerts);

	// Read in the array of vertices (an array of 3 floats)
	pPreviousChunk->bytesRead += fread(pObject->pVerts, 1, pPreviousChunk->length - pPreviousChunk->bytesRead, m_FilePointer);


The bytes read before the code is 8, after the call they are 20204. Any ideas as to where I went wrong? Thanks! Edit : oh and the length and numVerts are the same. (20204 length, 1683 numVerts)

Share this post


Link to post
Share on other sites
frob    44908
You didn't post enough code to know for certain what is going on.

Assuming m_file is of type basic_istream<char, char_traits>, and assuming previousChunk->length - previousChunk->bytesRead was correct, and assuming a POD data type, and assuming your file data matches the memory format in terms of padding and layout and endianness, and assuming no modifications by other threads or processes, and assuming the read was successful, and assuming other things that I am not thinking of right now because I'm feeling lazy, then the total should have been 20204 as you pointed out.

But that is a lot to assume.



I strongly suggest you go back and learn the language before writing 3D code.

There are many errors in your code block, and in the tutoral.

Your code makes several assumptions.

Using memset in c++ is generally a very bad thing. You are assuming that Vector3 is a POD structure. If for any reason it isn't, that code is a bug.

Instead, either the default constructor should initialize the Vector3 objects or you should call a member function to initialize them, as appropriate.

Alternatively, since you know you will be loading them, you don't need to bother setting them to a value when you know the value will immediately be overwritten.

Similarly, writing directly over the casted objects assumes you have direct knowledge of the underlying data structure, that it is a POD struct, that the Vector3 objects in the array have no padding between them or that the data in the file has that padding built in, or that previousChunk->length - previousChunk->bytesRead is equal to sizeof(Vector3) * object->numVerts. Without a deeper understanding of the code any of those could be false.

The safer alternative is to process each one individually, setting the objects individually.

You are also making an assumption that the read was successful. Depending on your stream flags a failed read may or may not throw an exception, so you should probably check the stream's failbit after the read.



I strongly suggest you turn to a good book and learn the c++ language, and put this project on hold until you understand what you are doing.

Share this post


Link to post
Share on other sites
p0is0n    100
Apologies for the lack of information.

m_file is declared as :
std::ifstream m_file;

The code I origonally had was :

object->verts.resize(object->numVerts);
//Read in the vertices
for(int i = 0; i < object->numVerts; i ++)
{
float x, y, z = 0.0f;
m_file.read(reinterpret_cast<char*>(&x), sizeof(float));
previousChunk->bytesRead += m_file.gcount();
m_file.read(reinterpret_cast<char*>(&y), sizeof(float));
previousChunk->bytesRead += m_file.gcount();
m_file.read(reinterpret_cast<char*>(&z), sizeof(float));
previousChunk->bytesRead += m_file.gcount();

//Change axis to correct order
object->verts[i].x = x;
object->verts[i].y = z;
object->verts[i].z = -y;
}




object->numVerts is the number of vertices
object->verts is an std::vector<Vector3>
Vector3 is default initialised to (0,0,0)

previousChunk is a chunk from the file.

The problem is that the bytesRead is set at 8 before the code is entered,
and the number of vertices is 1683.
One would assume that 1683 (number of vertives) * (4 (the sizeof(float)) * 3 (x,y,z)
would lead to getting 20196 which if 8 is added (the initial value of bytesRead) gives : 20204 (the desired value).

The value I get in bytes read is 96 (minus the 8 gives 88 bytes read).

Normally I would never use memset or initialise a structure using struct foo = {0}; It just seemed like narrowing down the code to the closest similarity would help someone to see a problem. (Obviously a mistake).

I had totally forgotten about the failbits, It turns out that after reading the first 88 bytes the eof and fail bits are set.

The file being read is exactly the same, and the values for x,y and z are the same up until 88 bytes have been read (7 vectors and a float).

If anybody knows why the fread way works perfectly and my way breaks any advice would be much appreciated.

Oh and CVector3 (used in the fread way) is defined as :
class CVector3
{
public:
float x, y, z;
};

Thanks for the advice frob.

Share this post


Link to post
Share on other sites
p0is0n    100
Hi, yes the code I use for opening the file is:

//Open the file
m_file.open(filename.c_str(), std::ios::in, std::ios::binary);

Forgot to include this in the previous post, thanks.

Edit : ****, that's not right is it. It should be :
//Open the file
m_file.open(filename.c_str(), std::ios::in | std::ios::binary);

Works now. Sorry for being an idiot!

Share this post


Link to post
Share on other sites
p0is0n    100
Ok, I think i'm being stupid again and completely missing something.

The code I have seems to work for everything apart from strings (std::string).

char name[255] = {0};
m_file.read(name, length);
This fills the char array with the desired filename (read from a binary file using std::istream).

I was wondering what I would need to do to have say :
std::string name = "";
m_file.read(name, length);

The only way I seem to be able to get rid of the compiler errors is to use :
std::string name = "";
m_file.read(reinterpret_cast<char*>(&name), length);

Which gives "" as the output even though the bytes are read from the file. I'm sure there is a very good reason for this, but what that reason is I don't know at the moment.

I'm guessing i'm completely overlooking something obvious and simple, but would just like to know if anyone knows how to accomplish what I am trying to do.

Any help/advice would be much appreciated.
Thanks!

Share this post


Link to post
Share on other sites
Zahlman    1682
Quote:
Original post by p0is0n
Ok, I think i'm being stupid again and completely missing something.

The code I have seems to work for everything apart from strings (std::string).

char name[255] = {0};
m_file.read(name, length);
This fills the char array with the desired filename (read from a binary file using std::istream).

I was wondering what I would need to do to have say :
std::string name = "";
m_file.read(name, length);

The only way I seem to be able to get rid of the compiler errors is to use :
std::string name = "";
m_file.read(reinterpret_cast<char*>(&name), length);

Which gives "" as the output even though the bytes are read from the file. I'm sure there is a very good reason for this, but what that reason is I don't know at the moment.

I'm guessing i'm completely overlooking something obvious and simple, but would just like to know if anyone knows how to accomplish what I am trying to do.

Any help/advice would be much appreciated.
Thanks!


You're not being stupid, but you are missing a hell of a lot. :)

You can only use .read() to read into POD structs. It only accepts a char* pointer, and you read into POD structs by casting the pointer. In effect, you give the instruction "write to memory one byte at a time, starting at the beginning of this object in memory".

A std::string does not qualify as a POD. Its data layout may contain different things depending on your implementation, and you're not entitled to know exactly what's in it. It certainly does not just contain the text "directly", starting at the beginning of the structure, because in C++, objects must have a known-at-compile-time size. Thus a std::string instance will generally manage a pointer to some kind of dynamic memory allocation. When you use .read() to read over (not "into", really) the std::string, you can easily overwrite this pointer and screw everything up.

That's the practical view. The official view given by the language standard is that absolutely anything is allowed to happen at this point. This is what we call Undefined Behaviour, and it's a very bad thing.

By the way, the .read() member function DOES NOT put a null terminator at the end of the data. In your case, this is not a problem because your buffer is initialized to all zeros. However, your buffer MUST have room for a null terminator at the end; since you declared a buffer of 255 chars, 'length' can be at most 254. And you should be aware that you cannot return the buffer from the function, either.

Anyway, back to std::strings.

If you want to read one word from the file, just use the operator>> overload for std::string:


m_file >> name;


If you want to read one line from the file - or in general, everything up to the first occurrance of a specific character, use the free function std::getline:


getline(m_file, name); // reads up to the first occurance of '\n'
getline(m_file, name, ','); // reads up to the first comma


If you want to read a specific number of characters from the file and create a string from them, you will need a temporary buffer, which you then use to either construct or assign the string:


// With a character count that is KNOWN AT COMPILE-TIME ONLY
char buffer[NAMES_ARE_ALWAYS_EXACTLY_THIS_LENGTH];
m_file.read(buffer, NAMES_ARE_ALWAYS_EXACTLY_THIS_LENGTH);

// Notice that in this case you DON'T need room for a null terminator, because
// you can tell the std::string how long the string is.

// Construction:
std::string name(buffer, buffer + NAMES_ARE_ALWAYS_EXACTLY_THIS_LENGTH);
// Assignment:
name.assign(buffer, buffer + NAMES_ARE_ALWAYS_EXACTLY_THIS_LENGTH);

// With a character count that is determined at runtime, or a known-at-compile-time length:
std::vector<char> buffer(length);
m_file.read(buffer, length);

// Construction:
std::string name(buffer.begin(), buffer.end());
// Assignment:
name.assign(buffer.begin(), buffer.end());

Share this post


Link to post
Share on other sites
p0is0n    100
Sorry, forgot to post back and say thanks.

I got it working using:

std::vector<char> buffer(length);
m_file.read(&*buffer.begin(), length);
std::string name = "";
name.assign(buffer,begin(), buffer.end());



Thanks a lot Zahlman for the help and for the clarification.

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