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

Library extension #62

Open
alecive opened this issue Feb 17, 2016 · 5 comments
Open

Library extension #62

alecive opened this issue Feb 17, 2016 · 5 comments

Comments

@alecive
Copy link
Contributor

alecive commented Feb 17, 2016

Hey @memononen ,

I really like this library, so thank you for your efforts in it! I am trying to extend it a bit in order to suit my needs. I would like to give you back my contributions disguised as pull requests and whatnot. Specifically, I am in the process of doing:

  • nsvgCopyGradient
  • nsvgCopyPaint
  • nsvgCopyPath
  • nsvgCopyShape
  • nsvgAppendShape (it concatenates two shapes in order to have a single picture)
  • nsvgTransformPath
  • nsvgTransformShape
  • ...

Would you be interested in including them in the library? I think that they can be useful also for other users (but if you are not interested I will keep them on my private copy).

@memononen
Copy link
Owner

Hi! These sounds like interesting additions. Yes, I'd be interested to add them.

@alecive
Copy link
Contributor Author

alecive commented Feb 17, 2016

Ok, great!
Let me then ask you some questions while I am implementing the methods (this is the main reason why I opened an issue and not directly a pull request). I am a C++ developer so some things are not that obvious to me.

First things first: is this nsvgCopyPath function ok with you?

NSVGpath* nsvgCopyPath(NSVGpath* _p)
{
    NSVGpath* res = (NSVGpath*)malloc(sizeof(NSVGpath));
    res->npts = _p->npts;
    res->pts  = (float*)malloc(res->npts*sizeof(float));

    for (int i = 0; i < res->npts; ++i)
    {
        res->pts[i] = _p->pts[i];
    }

    res->closed = _p->closed;

    for (int i = 0; i < 4; ++i)
    {
        res->bounds[i] = _p->bounds[i];
    }

    res->next = _p->next;

    return res;
}

With "ok with you" I mean if it's ok to have a function declaration like the one above or something different (for example, bool nsvgCopyPath(NSVGpath* _src, NSVGpath* _res). It's completely up to you. I rather prefer the first option because it saves the user the step of allocating memory for the path.

@memononen
Copy link
Owner

Copy should be int nsvgCopyPath(NSVGPath* dst, NSVGPath* src), the above function should be called nsvgDuplicatePath() as it returns a dupe :)

The above function is not in line how the rest of the lib is written (i.e. should not use those underscores), and public API should be more paranoid. Something like this:

NSVGpath* nsvgDuplicatePath(NSVGpath* p)
{
    NSVGpath* res = NULL;

    if (p == NULL)
        return NULL;

    res = malloc(sizeof(NSVGpath));
    if (res == NULL) goto error;
    memset(res, 0, sizeof(NSVGpath));

    res->pts = malloc(p->npts * sizeof(float) * 2);
    if (res->pts == NULL) goto error;
    memcpy(res->pts, p->pts, p->npts * sizeof(float) * 2);
    res->npts = p->npts;

    memcpy(res->bounds, p->bounds, sizeof(p->bounds));

    res->closed = p->closed;

    return res;

error:
    if (res != NULL) {
        free(res->puts);
        free(res);
    }
    return NULL;
}

void nsvgAddPathToShape(NSVGpath* s, NSVGpath* p)
{
    if (s != NULL && p != NULL) {
        p->next = s->paths;
        s->paths = p;
    }
}

I think it should not add the path to the linked list automatically.

@alecive
Copy link
Contributor Author

alecive commented Feb 18, 2016

Ok great! I will then proceed with nsvgDuplicatePath and open a pull request (just to warm us up).

Some questions to go on:

  • What do you mean with "I think it should not add the path to the linked list automatically"? My idea was not to implement an addPathToShape, but rather an addShapeToImage (but the mechanism is similar so the question remains)
  • In order for me to implement nsvgDuplicateShape, I need to implement also a duplicate method for NSVGgradient and NSVGpaint. I was thinking to not expose them to the user. Shall I call them nsvg__duplicateGradient then (like any other method that is not exposed)?
  • I have some problems with the union in NSVGpaint (never seen it before). How can I copy that?
  • In NSVGgradient, the variable stops is defined as NSVGgradientStop stops[1];. Is it correct to assume that this array is going to be generally with a size bigger than 1?

alecive added a commit to alecive/nanosvg that referenced this issue Feb 18, 2016
@memononen
Copy link
Owner

The nsvgDuplicatePath() function I made does not add the path to any shape's linked list. I would like the adding to be explicit, so that you can do things like duplicate a path and add it to another shape. Also, there should be a matching nsvgDeletePath() and nsvgRemovePathFromShape().

I don't think you need nsvg__duplicateGradient() just make nsvg__copyPaint(), which should handle copying the gradient too when need be (and also deal with existing gradient in dst paint).

Everything in the union will occupy the same memory location. So you can only use one of the values. This is done based on the type. If the type is NSVG_PAINT_LINEAR_GRADIENT or NSVG_PAINT_RADIAL_GRADIENT then the pointer is used. Take a look at nsvg__deletePaint().

The one sized array in gradient is old C trick for variable sized array, take a look at how it is allocated:
https://github.com/memononen/nanosvg/blob/master/src/nanosvg.h#L824

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants