tyoma / agge

Anti-Grain Evolution. 2D graphics engine for Speed and Quality in C++.
MIT License
117 stars 17 forks source link

ideas for code (part-0) #11

Open DaMilyutin opened 1 year ago

DaMilyutin commented 1 year ago

I can not insist here. It is your code. So if you want to look into them. If you agree on some of those updates I could make some smaller issues for myself and PRs. This issue is way too big to be done all at once.

So what I've noticed.

  1. What is target C++ standard for this library? In example with balls-async there was unique_ptr. So it looks like C++11 at least. So maybe one could shift code there. As I looked in class dash I saw C++2003 style. I understand you migrated it from agg. And probably have no time for being C++11 code idealist.

  2. In my experience this pattern will oftenly repeat: image

When I contemplated agg I wanted to make it more expressive and functional style-like. (Ex. similar to C++20 ranges) And looks like you also trying to write less types. The pattern in picture above is piping. So one can just overload binary operator of preference for it to mean "pipe source with generator (decorator?)". So, code above will look like one of examples depending on preference: line(19.5f*scale, 300.5f*scale, 319.5f*scale, 300.5f*scale)|dash_style|line_style line(19.5f*scale, 300.5f*scale, 319.5f*scale, 300.5f*scale)/dash_style/line_style line(19.5f*scale, 300.5f*scale, 319.5f*scale, 300.5f*scale)^dash_style^line_style line(19.5f*scale, 300.5f*scale, 319.5f*scale, 300.5f*scale)+dash_style+line_style line(19.5f*scale, 300.5f*scale, 319.5f*scale, 300.5f*scale)>>dash_style>>line_style line(19.5f*scale, 300.5f*scale, 319.5f*scale, 300.5f*scale)>dash_style>line_style line(19.5f*scale, 300.5f*scale, 319.5f*scale, 300.5f*scale)<=dash_style<=line_style

Looking at these now most distinctive is IMO "^". Operators +-*/% could be in parameters of line, |&% are hard to see. Shift and comparison may be toooo confusing here. And ^ stands out in this expression as natural separator making it easy to read.

Or (I like it less) one can add template method named ex .with: line(19.5f*scale, 300.5f*scale, 319.5f*scale, 300.5f*scale).with(dash_style).with(line_style).

Actually template overloading of operators without restrictions is unsafe. One should use sort of dispatch techniques or concepts. But this can be done right!

  1. in design of class image one can use universal reference in member declaration: GeneratorT &&_generator; and forwarding in ctor:
    template <typename SourceT, typename GeneratorT>
    inline path_generator_adapter<SourceT, GeneratorT>::path_generator_adapter(const SourceT &source, GeneratorT &&generator)
        : _source(source), _generator(std::forward<GeneratorT>(generator)), _state(initial)
    {   }

    and in assist (or operator of preference 😄):

    template <typename SourceT, typename GeneratorT>
    path_generator_adapter<SourceT, GeneratorT> assist(const SourceT &source, GeneratorT &&generator)
    {   return path_generator_adapter<SourceT, GeneratorT>(source, std::forward<GeneratorT>(generator));    }

    And default move and copy constructors.

Why bother? With this you can use on a fly construction like: line(19.5f*scale, 300.5f*scale, 319.5f*scale, 300.5f*scale)^create_some_custom_line_style(some_params)

  1. when looked at this part: image There can be piping too! Imagine code will look like:
    line(19.5f*scale, 300.5f*scale, 319.5f*scale, 300.5f*scale)
          ^dash().add_dash(1.0f*scale, 1.0f*scale).dash_start(0.5f*scale)
          ^line_style.width(1.0f*scale).set_cap(caps::butt());
  2. To my taste dash::remove_all_dashes() can be renamed as clear() when reused. Also, some guideline discourage duplication in class and method names. So, maybe dash().add(1.0f*scale, 1.0f*scale).start(0.5f*scale)?

I see those are public methods and there are "public but actually not for user" methods to generate vertexes. Will study further.

That's it by now.

BR, Daniel

tyoma commented 1 year ago

WOW, @DaMilyutin! I'm rarely looking at the issues here, as am mostly busy with https://github.com/tyoma/micro-profiler, so it was a pleasant surprise )) In fact, you got it right - i'm trying to keep it as close to functional paradigm as I can. The thing with agge (maybe agge.text and definitely not agge.async which is just an experiment as of yet) is that I intentionally kept it C++03-compliant for many reasons. Maybe these reasons are worth reconsidering. When I do C++, I prefer C++11 (even to newer flavors, when I'm not forced to). Generators and mutators (assist) can be turned into op-overloads, but my concern there was that sometimes mutation implies unbound memory allocations, while I wanted all my primitives to follow static allocation pattern (hence, the user-provided generator, rather than auto-constructed ones). Hit me up on linkedin (the link is in the bio) - we can converse there, as your comments are more related to the conceptual design rather than to specific issues.