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

Added mathods for finding/setting the bounding box of a path #17

Closed
wants to merge 1 commit into from

Conversation

kpym
Copy link
Contributor

@kpym kpym commented Dec 22, 2015

  • svgpath(__your_path__).toViewBoxString(d) return a string encoding the minimal box containing the path : "min max width height" (with precision d).
    The default precision is 2.
  • svgpath('__your_path__').toBox('0 0 100 100 slice xMinYMax') put the path in a box controlled by parameters similar to preserveAspectRatio.

There is also a method svgpath('__your_path__')getBoundingBox() that return a Box object and that is used by the previous two methods.

@puzrin
Copy link
Member

puzrin commented Dec 22, 2015

I don't like suggested API change. It becomes not consistent.

@kpym
Copy link
Contributor Author

kpym commented Dec 22, 2015

Could you be more precise ?

@puzrin
Copy link
Member

puzrin commented Dec 22, 2015

I'd add .bbox() -> [ x, y, width, height ]. Isn't it enought?

@kpym
Copy link
Contributor Author

kpym commented Dec 22, 2015

My first version was like your .bbox(), but all the complexe calculations in this case goes to svgpath library. It is better to have Box object.

@puzrin
Copy link
Member

puzrin commented Dec 22, 2015

Could you be more precise ?

Suggested methods goes too far from original concept - (short names + chaining + non trivial ops). Also i'm not sure that formatting to viewbox should be part of this lib.

@kpym
Copy link
Contributor Author

kpym commented Dec 22, 2015

If you don't want to have inBox method in this library, no problem. I'll keep it for me ;)

@kpym
Copy link
Contributor Author

kpym commented Dec 22, 2015

I don't like to put the precision in .toViewBoxString(d) but this is how your library works : .round() do not save the precision in the object in a way to be used by other output methods. I would prefer to have .round(2).toViewBoxString() but path with integer coordinates can have a complex bounding box.

@puzrin
Copy link
Member

puzrin commented Dec 22, 2015

I'd like to avoid specific data formats. Let's keep it aside if simple data is not enougth for your needs.

I would prefer to have .round(2).toViewBoxString()

  • I see no principal problem to remember last used round(x) in internal property. Then it will be available in .bbox()
  • Also, you can keep BoxFormat(Array) separately from svgpath for formatting.

I don't reject possibility of API change. But it's better to discuss those first.

@kpym
Copy link
Contributor Author

kpym commented Dec 22, 2015

I see no principal problem to remember last used round(x) in internal property. Then it will be available in .bbox().

Yes this is how it should be IMO. But now round() is applied to the path, not saved. And this is a big change. For me the methods like .abs(), .rel() and .round() should only be saved in the object and not applied until a final output controlled by this parameters or until .apply().
For example I would like to add methods like .keepSpaces() (to not reduce spaces in the output) or .withComma() to put comma in between coordinates in the .toString() ...
This will also make automatic optimisation for .abs().rel().abs().rel().

@puzrin
Copy link
Member

puzrin commented Dec 22, 2015

Create a separate issue with examples of all new methods you wish (without implementation details)

@kpym
Copy link
Contributor Author

kpym commented Dec 22, 2015

Would you be happy with this two methods :

  • .bbox() -> [ x, y, width, height ] : such that the returned array has two attached methods .round() and .toString() in a way that we can chain svgpath(...).bbox().round(2).toString().
  • .tobox(x, y, width, height [,params]) -> self : translate and scale the path to put it in a box with optional string params like 'slice xMinYMax'.

@puzrin
Copy link
Member

puzrin commented Dec 22, 2015

.bbox() -> [ x, y, width, height ] : such that the returned array has two attached methods .round() and .toString() in a way that we can chain svgpath(...).bbox().round(2).toString().

I don't like idea of attached methods. Also, output probably could be [ x1, y1, x2, y2 ] - don't know what is better.

.tobox(x, y, width, height [,params]) -> self : translate and scale the path to put it in a box with optional string params like 'slice xMinYMax'.

That looks very specific. Not sure someone else need it. Also, implementation is a simple math, and that will not add a lot of value.

@kpym
Copy link
Contributor Author

kpym commented Dec 22, 2015

That looks very specific. Not sure someone else need it. Also, implementation is a simple math, and that will not add a lot of value.

You can do every transform with .matrix but there are all other methods ... why not to have this one too ? If you see the code, it is not so simple to have all this power of how to put the path in the box.

@puzrin
Copy link
Member

puzrin commented Dec 22, 2015

why not to have this one too ? If you see the code, it is not so simple to have all this power of how to put the path in the box.

There are different approach to API design. I prefer micromodules/minimalistic. If i start to add everything, API will become a mess like sugarjs. My criterias for new methods are:

  • those should be useful for many people
  • those should do not trivial things (math, or require deep SVG knowledge)

@kpym
Copy link
Contributor Author

kpym commented Dec 22, 2015

There are different approach to API design. I prefer micromodules/minimalistic. If i start to add everything, API will become a mess like sugarjs. My criterias for new methods are:

  • those should be useful for many people
  • those should do not trivial things (math, or require deep SVG knowledge)

I agree with you. You can close this pull request I think.

@puzrin puzrin closed this Dec 22, 2015
@3rd-Eden
Copy link

Sad to see this PR be closed as this was just the functionality I was looking for. I needed to get the dimensions of a given path in order to calculate the correct transform scale so it would fit my given width and height restrictions.

@puzrin
Copy link
Member

puzrin commented Nov 23, 2016

I explained the reasons above. This PR suggested more than just simple bbox calc, and it's signature become a bit "strange" in context of this project. If you wish to do PR with something like bbox() -> [ x1, y1, x2, y2 ], it will be accepted.

Or check dependents of this project in npm registry, there are bbox calculation somewhere.

@kpym
Copy link
Contributor Author

kpym commented Nov 23, 2016

@3rd-Eden you can check my fork of svgpath. It has .inbox command that scales and translates a path to fit a given box.

@3rd-Eden
Copy link

Sweet, exactly what i was looking for as i also needed it in browser and less strict parsing :p!

On Nov 23, 2016, at 8:59 PM, Kroum Tzanev notifications@github.com wrote:

@3rd-Eden you can check my fork of svgpath. It has .inbox command that scales and translates a path to fit a given box.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@puzrin
Copy link
Member

puzrin commented Nov 24, 2016

Ah, yes. I can also recommend to take a look at @kpym's fork https://github.com/kpym/SVGPathy. He helped a lot to improve math about arcs and he really know what he does.

@markvital
Copy link

Are there any plans to merge this?

@puzrin
Copy link
Member

puzrin commented May 29, 2020

As is - no.

@markvital
Copy link

In SVG specification bounding box is defined as [x, y, width, height], where width >= 0, height >= 0
So it would be the best to keep it the same way for consistency.
[x1, y1, y2, y2] format is better suited for some algorithms (like finding minimum/maximum or intersections), but it's easy to convert between first and second using elementary math.

@markvital
Copy link

As is - no.
I see, makes sense.

@puzrin
Copy link
Member

puzrin commented May 29, 2020

@markvital See #17 (comment)

@ktzanev as far as i remember, you created fork because had no time (or no wish) to rewrite your code for alternate signaure. What do you think now? Potentially, i would be interested to add .bbox() => [ x, y, w, h ] method. I have 2 questions for you:

  • Is such signature "useable" (in your projects), or BBox class sill better?
  • If you have no time/interest, can you give permission to take math from this PR? Or i could prepare code and ping you for recommit PR from your account.

@kpym
Copy link
Contributor Author

kpym commented May 29, 2020

@puzrin I forked this library because at the time my requests were very specific and didn't fit very well with your needs, I think.

The code of my fork has diverged from the original over time, but if you can/wish to use some of my code, you are welcome to do so of course.

I'm sorry, but I didn't quite understand your questions... May be because I have to dive in my own code to remember what I've done ;)

@puzrin
Copy link
Member

puzrin commented May 29, 2020

I'm sorry, but I didn't quite understand your questions... May be because I have to dive in my own code to remember what I've done ;)

In PR you returned BBox() class instance instead of more simple data (array with 4 elements) https://github.com/fontello/svgpath/pull/17/files#diff-54d0d5c3efa392c0744777d393d004d1R8. From my point of view it was too complicated "in context of svgpath".

I'd prefer .bbox() => [ x, y, w, h ].

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

Successfully merging this pull request may close these issues.

None yet

4 participants