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

Scaling and cropping from the centre #41

Closed
duncangeere opened this issue Jul 11, 2020 · 5 comments
Closed

Scaling and cropping from the centre #41

duncangeere opened this issue Jul 11, 2020 · 5 comments

Comments

@duncangeere
Copy link
Contributor

I feel like it should be easier to use the centre of the geometries as the origin for scale and crop.

Rotate already uses the centre as the origin by default, which I think is correct. I feel like scale should also operate from the centre by default as this would be the most common use case during pen plotting. If you think this might confuse users, allowing --origin center might be a good compromise. One can, of course, always use --center in the write command to fix this, but I don't think that giving users an alternative here is a bad idea.

With crop it's a little trickier because it uses X and Y rather than --origin. But again - I feel like a common use case would be cropping in from all the edges evenly, and this is currently very difficult to calculate manually. Might be worth thinking about refactoring this to use --origin too, so it matches the other commands?

@abey79
Copy link
Owner

abey79 commented Jul 11, 2020

Currently, scale's origin behaves the same as rotate: it uses the bounding box's center by default, unless --origin is provided. So does skew. So I think this is more or less consistant.

The one weirdness about the scale command is the behaviour switch when using --to. This is both kind of tricky and an important feature (at least I'm using it very often myself). This has already been pointed as confusing (see #17). I'm currently leaning toward splitting the command into a scale and scaleto, to make things clearer. I'd be interested to have your take on this.

As for cropping, my feeling is that a new trim command which would crop based on an offset w.r.t. the current bounding box and leave crop as is. Would that make sense to you?

@duncangeere
Copy link
Contributor Author

duncangeere commented Jul 12, 2020

Currently, scale's origin behaves the same as rotate: it uses the bounding box's center by default, unless --origin is provided. So does skew. So I think this is more or less consistant.

Ah! I was basing my comment on the --help text, so it might be good to specify that in scale and skew in the same way it is in the help text for rotate.

The one weirdness about the scale command is the behaviour switch when using --to. This is both kind of tricky and an important feature (at least I'm using it very often myself). This has already been pointed as confusing (see #17). I'm currently leaning toward splitting the command into a scale and scaleto, to make things clearer. I'd be interested to have your take on this.

I like this suggestion. I agree that scaleto is a far more common use case, so you might consider whether you need scale in there at all. I guess it doesn't hurt to have it for completeness.

As for cropping, my feeling is that a new trim command which would crop based on an offset w.r.t. the current bounding box and leave crop as is. Would that make sense to you?

I guess I'm wondering what the use case for crop is in its current form. I can't see it being very commonly used, though again it doesn't hurt to have it for completeness. I like the idea of trim, and would love to see that implemented, as well as perhaps an expand that does the opposite - adding a margin on all sides outward from the bounding box of the geometry?

@abey79 abey79 mentioned this issue Jul 12, 2020
@abey79
Copy link
Owner

abey79 commented Jul 12, 2020

I like this suggestion. I agree that scaleto is a far more common use case, so you might consider whether you need scale in there at all. I guess it doesn't hurt to have it for completeness.

I actual use scale quite a bit when doing generative stuff, which I usually package as vpype plug-ins. In generative code, I use arbitrary units (eg. circles with diameter of 1) and then use scale with units (eg. scale 1cm 1cm, now my circles have diameters of 1cm). This is actually really convenient, but I will agree that this is rather advanced workflow.

I like the idea of trim, and would love to see that implemented

#43

as well as perhaps an expand that does the opposite - adding a margin on all sides outward from the bounding box of the geometry?

This is currently not possible without a change to the data model. Currently, vpype doesn't know of a bounding box per se (independently of the actual vector content). So for vpype, whenever the bounding box is considered (e.g. for scaleto, to resize correctly), it uses the smallest rectangle that fits whatever vector data is in the pipeline.

Of course, SVG does implement a specific bounding box (with the viewPort, width, height stuff), so this information is actually lost in read. I've considered adding that information into vpype's data model, so that a read/write sequence would maintain the actual margins, but this comes with a significant increase in complexity for the user, for examples:

  • Commands that uses the bounding box may have unexpected results depending on the bounding box defined in input SVG.
  • Pipeline that don't start with read (eg. generative plug-ins) have no initial bounding box defined.
  • Should bounding box be adjusted when new vectors are added outside of it? In some case it'd make sense, in some other it wouldn't.
  • New commands should be added to let the user interact, modify, etc. the bounding box.
  • etc.

As a result, I've been avoiding doing this modification until a very clear workflow emerges that requires it.

@duncangeere
Copy link
Contributor Author

Gotcha, that all makes sense. I think your approach on the bounding box stuff is probably smart! I'll close this issue now as the main outcome - adding trim - is now covered in #43.

@abey79
Copy link
Owner

abey79 commented Jul 12, 2020

One more thing: I consider the CLI --help text to be an integral part of the documentation. As a matter of fact, it's used to generate the Reference section of the doc. Feedback is obviously welcome, and feel free to edit them in your PRs. This involves modifying comments in the actual code, so you would need to locate the corresponding function in vpype_cli/, eg. for scale (in vpype_cli/transforms.py):

# noinspection PyShadowingNames
@cli.command(group="Transforms")
@click.argument("scale", nargs=2, type=LengthType())
@click.option(
    "-l",
    "--layer",
    type=LayerType(accept_multiple=True),
    default="all",
    help="Target layer(s).",
)
# ... shortened ...
@global_processor
def scale(
    vector_data: VectorData,
    scale: Tuple[float, float],
    layer: Union[int, List[int]],
    absolute: bool,
    keep_proportions: bool,
    origin_coords: Tuple[float, float],
):
    """Scale the geometries.

    The origin used is the bounding box center, unless the `--origin` option is used.

    By default, the arguments are used as relative factors (e.g. `scale 2 2` make the
    geometries twice as big in both dimensions). With `--to`, the arguments are interpreted as
    the final size. In this case, arguments understand the supported units (e.g.
    `scale --to 10cm 10cm`).

    By default, act on all layers. If one or more layer IDs are provided with the `--layer`
    option, only these layers will be affected. In this case, the bounding box is that of the
    listed layers.
    """

    # these are the layers we want to act on
    layer_ids = multiple_to_layer_ids(layer, vector_data)
    bounds = vector_data.bounds(layer_ids)

    if not bounds:
        return vector_data

    # ... shortened ...

    return vector_data

You can tell a function actually implements a command by the top @cli.command(group="Transforms") decorator, usually followed by a whole lot of @click.argument and @click.option. The big comment in the beginning of the function is used for the corresponding command's --help text. You'd just need to maintain the existing indentation and line length (95 characters). No Markdown/reStructuredText formatting command are allowed either, as the text is displayed (more or less) verbatim in the CLI. The top line of the comment is used as summary in various places (command list, etc.)

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