Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Build on Windows - incorrectly saved .ppm format #9

Open
cyclone125 opened this issue Feb 15, 2019 · 6 comments

Comments

@cyclone125
Copy link

@cyclone125 cyclone125 commented Feb 15, 2019

Hi!
First of all thank you for great tutorials on 3D graphics.

There is an issue when building project on Windows with GCC. In initial commit in file tinyraycaster.cpp in function drop_ppm_image you have:

std::ofstream ofs(filename);
ofs << "P6\n" << w << " " << h << "\n255\n";

On Windows GCC replaces \n in output file with platform specific code 0x0D0A, what is incorrect (it should be 0x0A in the file). This result in incorrect *.ppm image representation.
This happens because of opening file in "text" mode. To fix this, file should be opened in "binary" mode, for example like this:

std::ofstream ofs;
ofs.open(filename, std::ofstream::out | std::ofstream::binary);
ofs << "P6\n" << w << " " << h << "\n255\n";

Then everything works correctly. I think this patch would not affect other platforms.

@ssloy

This comment has been minimized.

Copy link
Owner

@ssloy ssloy commented Feb 15, 2019

Yes, this issue had already been adressed by this commit.

@Liquix

This comment has been minimized.

Copy link

@Liquix Liquix commented Feb 19, 2019

I love your writeups and walkthroughs! Thanks for putting them together. However, a big obstacle for beginners is that the first code snippets (in both tinyraycaster and tinyraytracer repositories) do not produce matching output when copied/pasted and compiled on Windows machines. I understand that you and many other enthusiasts develop on Linux, but there is nowhere (that I could find) where the reader is warned that the code must be modified in order to follow along on Windows. A bold note at the beginning or quick reminder under the code snippet would be much appreciated. Thanks again for all your hard work.

@ssloy

This comment has been minimized.

Copy link
Owner

@ssloy ssloy commented Feb 19, 2019

From my experience, bold reminders are ignored by most readers. That is why I keep this current issue open even if in later commits the bug was fixed.

@cyclone125

This comment has been minimized.

Copy link
Author

@cyclone125 cyclone125 commented Feb 19, 2019

It's not only about ignoring reminders. It's about someone, who is trying to start from the very beginning step by step, as you recommend in your tutorials, and taking the link to initial commit to try it himself. Then he realizes that output is different comparing to what it should be and it's unclear why, and it takes some time to find where the problem hides. Also for the beginner it's not trivial to find the problem himself.
It's not criticism, just some thoughts. Also I don't know if there is a really good way to fix this in initial commit.

@ssloy

This comment has been minimized.

Copy link
Owner

@ssloy ssloy commented Feb 19, 2019

I do agree with you, this is problematic, but I do not see a satisfactory solution to the problem. I do create bugs (a lot). This repository is a real history of one of my saturdays, not something I prepared for months before. While modifying the very first commit is possible, I do not master git sufficiently to feel a firm ground under my feet with this kind of solution.

@cyclone125

This comment has been minimized.

Copy link
Author

@cyclone125 cyclone125 commented Feb 19, 2019

Just notice: I think, this is possible to edit it directly on github just as text file, but I can't guarantee consequences.
Summing up, I think that small reminder in README file about building on different platforms (including this issue) would be helpful anyway for those people, who would find it. :)
Thank you again for you work and sharing knowledge.

@ssloy ssloy pinned this issue Feb 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.