c++ CLI Ascii image renderer

Question

I am wondering if this is written well or not, I am just a beginner and would like some feedback what should I improve on. Thanks.

#define STB_IMAGE_IMPLEMENTATION
#include<iostream>
#include "stb_image.h"
#include "vector"
#define LOG(x) std::cout << x << std::endl
const char* density = "#$lho,. ";
struct Vec3{
 float x, y, z;
 float Average(){
 return (x+y+z)/3.0f;
 }
 Vec3& operator/(float other){
 x /= other;
 y /= other;
 z /= other;
 return *this;
 }
};
std::ostream& operator<<(std::ostream& stream, const Vec3& other){
 stream << other.x << ", " << other.y << ", " << other.z;
 return stream;
}
int main(){
 std::ios_base::sync_with_stdio(false); 
 int w, h, c;
 unsigned char *data = stbi_load("image.jfif", &w, &h, &c, STBI_rgb);
 std::vector<Vec3> vec;
 for (int i = 0; i < w*h*c; i+=3)
 vec.push_back(Vec3{ float(data[i]), float(data[i+1]), float(data[i+2]) });
 
 int counter = 0;
 for (Vec3 v : vec){
 if (counter%w == 0) {
 std::cout << "\n";
 }
 std::cout << density[ int((v/255).Average()*(strlen(density)-1)) ] << " ";
 counter++;
 }
 LOG(counter);
 stbi_image_free(data);
 
 return 0;
}

• pacmaninbw: If you wrote the code in the header file stb_image.h, please add that as well, if you didn't, please provide a link to the code. Is the include file vector the C++ standard vector header?

Answer 1

Overview.

This is probably not your fault but the stbi interface is badly designed C++. This is more like a C interface.

The problem here are:

• The "Resource Allocation"/ "Resource Release" done manually
• There is no excapsulation of the data.

The resource alocation is the big thing to me (you need to use RAII). As a consequence, the code is not exception safe, and you are going to leak the resource in any non-trivial application.

Additionally, the data is actually 4 items.

int w, h, c;
unsigned char *data;

But they are not protected from misuse and thus likely to be messed up.

---

Not sure running across the data converting char to float then running across the data again is it worth it. Simple run across the data and print it computing an average as you go.

Code Review

What does this do.

#define STB_IMAGE_IMPLEMENTATION

It should be made clear why you are doing this.

---

Please be neat in your code.

#include<iostream>

Add an extra space after the include.

---

Vector is standard library. It is probably not something you want to define yourself (as implied by the quotes). Or this should use <> around vector to show this is a standard library.

#include "vector"

---

Don't use macros for this:

#define LOG(x) std::cout << x << std::endl

Macros should normally be reserved for designed machine/architecture differences. Not writing pseudo functions.

 template<typename T>
 inline void log(T const& value) {std::cout << value << std::endl;}

---

Don't use std::endl.

std::cout << value << std::endl;

This outputs a new line then flushes the buffer. This will make using the streams very inefficient (and if used a lot will significantly deteriorate the performance of an application). I would say that if you want to flush the buffer you should explicitly do it.

If you want your logging to force a flush fine then keep it.

But normally you sould simply output a new line.

std::cout << value << "\n";

Then if you actually need to flush then ask the engineer to explicitly flush it at important points (but usually the system does it at the correct times).

If this is for debugging use std::cerr this will auto flush (as it has no buffer to flush).

---

That's a short density string.

const char* density = "#$lho,. ";

---

Sure. But is "Average" the best function for this?

 float Average(){
 return (x+y+z)/3.0f;
 }

See this video where he discusses some alternatives:

---

Yes.

 std::ios_base::sync_with_stdio(false); 

---

How I would do it

// I am going to use the same type of class for a point
// You did but I am going to make the data points
// the same as the data you get from the stbi library
// so there is no need to do any conversion.
struct Vec3
{
 unsigned char x, y, z;
 float Average() const
 {
 return (x+y+z)/3.0f;
 }
};
// You are effectively manipulating an image.
// So lets encapsulate that data in its own class.
class Image
{
 int w;
 int h;
 int c;
 unsigned char *data;
 public:
 // We know that we are doing resource management
 // So we need a constructor and destructor to
 // correctly handle the resource.
 //
 // Note we could do something fancy with a smart
 // pointer. But I think that is overkill for now.
 // But may be worth thinking about down the road.
 Image(std::string const& fileName)
 {
 data = stbi_load(fileName.c_str(), &w, &h, &c, STBI_rgb);
 }
 ~Image()
 {
 stbi_image_free(data);
 }
 // Need to think about the rule of 3/5
 // So I am going to delete the copy operations.
 // You may do something more sophisticated in the
 // long run.
 Image(Image const&) = delete;
 Image& operator=(Image const&) = delete;
 // The only other operation you do is scan over the
 // data for the image. This is usually done via iterators
 // in C++. I am going to do the simplist iterator I can
 // probably not super exactly compliant but it will work
 // for this situation. You should fix up to be better.
 struct Iterator
 {
 // Really the iterator is simply a pointer to
 // a location in the raw data of them image.
 // So that is all we need to store.
 unsigned char* data;
 // Moving the iterator you want to move by
 // three data points (as the three points represent
 // one pixel (I believe)).
 Iterator& operator++() {
 data+=3;
 return *this;
 }
 // You can add post increment and or the decrement
 // operators.
 // This is the operator that gives you back a reference
 // to one point in the data.
 // We simply convert it to be a pointer of the correct
 // type and then convert it to a reference before
 // returning.
 Vec3& operator*()
 {
 return *reinterpret_cast<Vec3*>(data);
 }
 // The main comparison for iterators. Is checking
 // if they are not equal. You should probably also
 // test for equivelance.
 bool operator!=(Iterator const& rhs) const
 {
 return data != rhs.data;
 }
 };
 // In C++ ranges are defined by the beginning
 // and one past the end. So we create iterators
 // at these points.
 Iterator begin() {return Iterator{data};}
 Iterator end() {return Iterator{data + w*h*c};}
};
int main()
{
 Image image("image.jfif");
 for (auto const& point: image) {
 std::cout << point.Average() << "\n";
 }
}

---

Side nots:

The range based for() uses the begin() and end() methods on an object to iterat across the range. So the above for loop is actually equivelent to:

 {
 auto begin = std::begin(image); // this by default calls image.begin().
 auto end = std::end(image); // this by default calls image.end().
 for (auto loop = begin; loop != end; ++loop) {
 auto const& point = *loop;
 std::cout << point.Average() << "\n";
 }
 }

This shows that we are using all the methods I define above. begin() and end() to get the range. operator++() to increment the iterator and operator!=() to check we are not at the end and finally operator*() to get a reference to a point.

• user673679: well... stbimage is a C library: github.com/nothings/stb
• Loki Astari: @user673679 Well that explains why it is bad C++. But that does not excuse the OP from writing C in a C++ program. All resource allocation should be correctly wrapped in a class to prevent leaks.