Intermediate representation for layered draw #9
Conversation
|
Thanks for the PR! It looks like you are making good progress. Your comments are very helpful to understand the changes. I know the existing code could probably use more comments and better variable naming. Anyway, once you get the intermediate representation fully working, I am assuming you are planning to integrate it into the existing functions. Just let me know when you are ready for a review or if there is anything I could help with. |
|
I think I've found a good signature to use and wanted to ask your take on it, as it's similar but not exactly the same as what you had: struct Options {
size_t width = 0; // Width in terminal characters. Set to 0 for automatic size based on terminal.
size_t height = 0; // Height in terminal characters. Set to 0 for automatic size based on terminal.
struct Axis {
double min = 0; // Start of axis. Set to 0 for automatic size based on data.
double max = 0; // End of axis. Set to 0 for automatic size based on data.
bool drawn = true;
bool label = true; // TODO: allow setting custom label string?
bool ticks = true;
bool tick_labels = true;
units_type tick_label_type = units_fracts;
};
Axis x = {};
Axis y = {};
type_type character_set = type_braille;
style_type style = style_light;
ColorBits color_type = ColorBits::e4; // bit depth of color representation
bool validate = true; // validate sizes for graph draw
bool border = false; // draw border around the graph
/* 5 bytes padding */
// graph-specific options
struct Histogram {
size_t bar_width = 1; // size of each bar in x-axis (in data points, e.g. braille has width of 2 per terminal character)
} histogram;
std::ostream &ostr = std::cout;
const char* title = nullptr;
};
struct Fragment {
Color color;
uint8_t data; // stores up to 8 data points or up to 255 values
};
using Texture = std::unique_ptr<std::vector<Fragment>>;
auto histogram_experimental(const std::vector<T>& data, const Options& options = {}, const Color& color = {color_red}) -> Texture&&;
auto histogram_experimental(const std::vector<T>& data, Texture&& texture, const Options& options = {}, const Color& color = {color_red}) -> Texture&&;
auto histogram_experimental(const std::vector<std::vector<T>>& datasets, const Options& options = {}, const std::vector<Color>& colors = {}) -> Texture&&;
auto histogram_experimental(const std::vector<std::vector<T>>& datasets, Texture&& texture, const Options& options = {}, const std::vector<Color>& colors = {}) -> Texture&&;
|
Yes, I would love to use
Nice, allowing users to configure the options per axis should be a big improvement. Originally, I thought it would be good to put the options that users would most likely want to change in the function signature, but this will likely make the libraries easier to use. I have wanted to do something similar with the tables library, to allow users to set options per table column or row. Anyway, note that the
Should this Here is a slightly simplified version of your proposal, which includes all of the existing function signatures that would need to be updated: union Color {
color_type color4; // 4-bit color
uint8_t color8; // 8-bit color
struct {
uint8_t red;
uint8_t green;
uint8_t blue;
} color24; // 24-bit true color
};
struct Axis {
long double min = 0;
long double max = 0;
bool labels = true;
bool ticks = true;
bool units_label = true;
units_type units = units_fracts;
};
struct Options {
size_t width = 0;
size_t height = 0;
Axis x = {};
Axis y = {};
bool border = false;
bool axis = true;
type_type type = type_braille;
// plot_type plot = plot_scatter;
mark_type mark = mark_dot;
// graph_type graph = graph_dot;
const char *title = nullptr; // Maybe: string &title;
style_type style = style_light;
Color color = color_red;
ostream &ostr = cout;
bool check = true;
// Internal // Is this needed?
// size_t bar_width = 1;
};
struct Fragment {
Color color;
uint8_t data;
};
inline int graph(const vector<vector<Fragment>> &texture, const Options &options);
template <typename T>
int histogram(const T &array, const Options &options = {});
template <typename T>
int histogram(const size_t rows, T *array, const Options &options = {});
template <typename T>
int plots(const T &arrays, const Options &options = {}, const vector<Color> &colors = {});
template <typename T>
int plot(const T &array, const Options &options = {});
template <typename T>
int plot(const size_t rows, T **array, const Options &options = {});
template <typename T>
int functions(const size_t numfunctions, function<T(T)> functions[], const Options &options = {}, const vector<Color> &colors = {});
template <typename T>
int function(const function<T(T)> &afunction, const Options &options = {});
template <typename T>
int function(T afunction(T), const Options &options = {});I tried to retain my existing names as much as possible, to make it easier for users to upgrade. |
|
The bar width was basically just a placeholder for a graph-specific option. Thought it might be nice to draw each histogram bar 2 characters wide in some cases.
I think it's actually better to leave color in the parameter, as users may wanna reuse option structs from before and only change color + data. You are right that mixing histograms does not make too much sense. It is pretty much just a proof of concept for mixing graphs and I should have probably used plots instead. As for the texture parameter, I would leave it in there to make it easier to layer the graph draws (both internally and for users not wanting to create a multi-dimensional array of input data). Regarding textures, is there a reason you prefer I have added the option |
Yeah, I used a 2D vector mainly for indexing convenience. Is there a reason you prefer the 1D version? Feel free to use the 1D version if it does not add too much complexity.
Nice, that will be a great improvement. The colorful example on the README (the visual result of k-means clustering) was created before this library supported multidimensional arrays, so I actually had to implement a custom version of |
|
I pushed a commit to the main branch to add support for some new block types. I do not believe this will conflict with any of your changes, but it does slightly change how it outputs the graph characters. I also added a new function to generate the color strings, which might be helpful. See ba19fd9 for the full changes. |
|
They won't cause any conflict, since I've not yet implemented proper drawing. Gonna use those once I do! I've put some more thought into the intermediate representation and decided to put both I've removed the histogram portion for now, as it makes much more sense to prototype with simple plots instead. One thing I'm not quite sure about is how the Braille characters should be indexed.. |
The Braille characters are currently listed in codepoint order in the library. See the Wikipedia page and the existing library code: Table-and-Graph-Libs/graphs.hpp Lines 82 to 83 in 2087d56 Table-and-Graph-Libs/graphs.hpp Lines 726 to 728 in 2087d56 |
|
The hex value of each dot really threw me off.. I had falsely assumed it was linear, i.e. Anyways, the two functions to showcase how to perform layered draws are Of course, this only contains the code for drawing the plot data, not the axis or other fluff. I reckon that should mostly be copy-able from the current implementation, as they get draw at the very end. Please do check out the current implementation! |
Thanks for all your work on this. Overall, it looks great! I can add some inline comments, if you are ready for that.
The other
Feel free to break it up into multiple PRs, if that is easier for you. In that case, we should just keep your |
Sounds good! As a side note, do you know why exactly the test on ubuntu 20.04 with clang++ fails? It seems to be a compiler-internal error with the |
It started failing on the main branch as well after ba19fd9, so I had to update the CI to ignore it. I would not worry about this too much, as GitHub is removing support for Ubuntu 20.04 in a few months anyway. It looks like this older version of Clang is just busted, as I can reproduce the issue on my system with a Ubuntu 20.04 VM. |
| struct Options { | ||
| size_t width = 0; // Width in terminal characters. Set to 0 for automatic size based on terminal. | ||
| size_t height = 0; // Height in terminal characters. Set to 0 for automatic size based on terminal. | ||
|
|
||
| Axis x = {}; | ||
| Axis y = {}; | ||
|
|
||
| type_type character_set = type_braille; | ||
| plot_type plot = plot_scatter; | ||
| style_type style = style_light; | ||
| graph_type graph = graph_dot; | ||
|
|
||
| std::string title; | ||
| std::ostream &ostr = std::cout; | ||
|
|
||
| ColorBits color_type = ColorBits::e4; // bit depth of color representation | ||
| bool check = true; // validate sizes for graph draw | ||
| bool border = false; // draw border around the graph | ||
| bool draw_immediately = true; // draw graph immediately after creation. otherwise call draw/graph with the returned texture | ||
| }; |
There was a problem hiding this comment.
This new Option struct is missing some of the existing options. See #9 (comment) above.
There was a problem hiding this comment.
I was only missing bool axis when compared to the Options there right? I have added bool drawn = true to the Axis struct for that one
There was a problem hiding this comment.
You are missing the mark option as well. The plot and graph options are currently unused.
Interesting about allowing the axes to be enabled independently. That will require some changes to the graph function.
| const size_t index = x + y * options.width; | ||
| const auto& frag = texture[index]; | ||
| // begin color draw( | ||
| cout << outputcolor(frag.color.col_4); |
There was a problem hiding this comment.
We should probably update the outputcolor function to accept the entire Color struct.
There was a problem hiding this comment.
We would need to pass the color bits as well, so the signature could look something like this?
inline string outputcolor(Color color, ColorBits color_bits)Makes sense to copy, rather than passing references
There was a problem hiding this comment.
Sounds good to me! I usually keep the const quantifier unless of course I am planning to modify the argument values.
| // intermediate representation of a graph texture for ease of passing around | ||
| struct Intermediate { | ||
| // use a graph texture to draw into the terminal | ||
| inline void draw() { |
There was a problem hiding this comment.
I am guessing you would want to eventually replace this with the existing graph function, of course updating it to accept an Intermediate struct instead.
There was a problem hiding this comment.
Yup that's the idea. I didn't wanna taint any functions outside my experimental area yet, so I put it into the Intermediate itself.
I used the Pixel_B structure from #8 to create a
fragmentstruct utilizing unions instead of std::variant, due to the latter being larger in total size (3 vs 8 bytes). Alongside the character data bit field, it's only 4 bytes in size.The intermediate representation is really just a
std::vector<fragment>, which makes it very easy to work with.I've chosen fragment as a name, as every fragment can store multiple pixels, i.e. data points. This allows us to store the underlying fragment data using a 8-bit bit field to store either 255 different values per fragment or 8 different data point states (useful for the 2x4 braille), which completely decouples it from the character set. In this example, the bit field is used to store values from 0-8 as the bar character has that many states.
This is supposed to simply show the base idea of how I would imagine the intermediate representation to be built. Of course, I would still change the formatting to fit yours once you are happy with the state of the implementation.
It currently lacks color merging (and use of 24-bit color, for that matter), which is what I plan on doing next. After that comes the generalization of characters during draw, in order to work purely on "data points" rather than characters.