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

Feature : Flip Horizontal/Vertical tools #204

Closed
juliandescottes opened this issue Jul 14, 2014 · 8 comments
Closed

Feature : Flip Horizontal/Vertical tools #204

juliandescottes opened this issue Jul 14, 2014 · 8 comments
Labels

Comments

@juliandescottes
Copy link
Collaborator

With modifiers to apply to all layers, to all frames ?
How to switch between the two modes ? Modifier as well ?

Maybe we should have a convention in the tools modifiers :

  • ALT for alternate mode
  • SHIFT for all frames
  • CTRL for all layers

Might be tedious in the long run. Maybe provide toggle buttons on top of the toolbox that would switch to all-frames/all-layers mode all tools that support it. Such tools would have a small indicator on their icon to highlight that they're affected by the change.

@ashindlecker
Copy link

I like the idea of a toggle button on top of the toolbox. Was adding features to the movement tool to apply to all layers and/or all frames. And I'm noticing some repeating code working on the flip tool.

Perhaps 2 check boxes on the top to represent whether or not to apply to all layers, all frames, or all layers and frames?

@juliandescottes
Copy link
Collaborator Author

Hi !

If you're looking at the flip tool, you can actually check in src/js/snippets.js

There's a handy window.flip that I use from a bookmarklet and that performs a flip :)

@ashindlecker
Copy link

Oh wow wish I saw that before rolling my own, I'll definitely use yours! And thanks for the quick reply, hope I didn't wake you up or anything!

At the moment I'll try to implement them as a tool with modifiers to control multiple frames and layers.
Though I'd imagine in a future build you'd love a toggle for modifying multiple frames and layers right? :p

@juliandescottes
Copy link
Collaborator Author

Haha, you didn't wake me up don't worry, it's 3pm here.

And feel free to change the implementation, you can use mine for reference when it comes to the most obscure parts, such as the events you should trigger after applying the tool.

At the moment I'll try to implement them as a tool with modifiers to control multiple frames and
layers. Though I'd imagine in a future build you'd love a toggle for modifying multiple frames and
layers right? :p

Sounds perfect, we'll do the toggle later :)

Something I thought about is that the flip tool is actually a bit different from the existing tools. For a regular tool, you click on the tool, then you click somewhere on the drawing area, and the tool is applied.

Here with the flip, you should simply click on the flip icon and it flips the frame, without having to click on the drawing area. Which behavior do you have in mind ?

Ultimately I think we will separate tools (pen, bucket ...) from effects (flip, rotate, hue ...).

Regarding the implementation in snippets.js

Looking at the flipFrame function in snippets.js , I'm doing something pretty ugly :

clone.forEachPixel(function (color, x, y) {
  if (horizontal) {
    x = w-x-1;
  }
  if (vertical) {
    y = h-y-1;
  }
  frame.pixels[x][y] = color;
});
frame.version++; // <- this is ugly

The proper way to do this is to actually call the setPixel method on frame, which will automatically update the version of the frame. (FYI : the frame version is used for caching various renderings of the frame).

The proper way would be :

clone.forEachPixel(function (color, x, y) {
  if (horizontal) {
    x = w-x-1;
  }
  if (vertical) {
    y = h-y-1;
  }
  frame.setPixel(x, y, color);
});

It will be a bit slower than the dirty one ... but I would still go for the cleaner implementation here.

@ashindlecker
Copy link

The proper way to do this is to actually call the setPixel method on frame, which will automatically update the version of the frame. (FYI : the frame version is used for caching various renderings of the frame).

Oh phew I'm glad we're on the same page! I was tempted to change the snippet but was unsure if there was some secret code going on in there! :P

Something I thought about is that the flip tool is actually a bit different from the existing tools. For a regular tool, you click on the tool, then you click somewhere on the drawing area, and the tool is applied.

Here with the flip, you should simply click on the flip icon and it flips the frame, without having to click on the drawing area. Which behavior do you have in mind ?

I completely agree with you. I don't think the user should be forced to click on the canvas when it's not needed.

Do you think that an effect should be completely separate from the tools to where it's separate from the tool list? At the moment I have something like this:

alt-text

Maybe we could have a separate list of effects in a similar layout to the tool list. When a user clicks on something from effect list, it'll just call the effect method.

What do you think?

@juliandescottes
Copy link
Collaborator Author

Nice, you already managed to go quite far !

Maybe we could have a separate list of effects in a similar layout to the tool list.

Yes, I was thinking about having tabs to switch between tools and effects.
For now we can just split the list of tools from the list of effects (gap, separator ?).

Regarding the help text for the ALT modifier, by 'Down for Horizontal, Up for Vertical', do you mean : if ALT is pressed : horizontal flip ; if ALT is released : vertical flip ?

@ashindlecker
Copy link

Yes, I was thinking about having tabs to switch between tools and effects. For now we can just split the list of tools from the list of effects (gap, separator ?)

Ah I see what you mean now. That sounds do-able! For now just to make things easier perhaps a gap will do.

Regarding the help text for the ALT modifier, by 'Down for Horizontal, Up for Vertical', do you mean : if ALT is pressed : horizontal flip ; if ALT is released : vertical flip ?

Yes sir. I'll definitely change what I have since I'm sure we can both agree that "Pressed" and "Released" sounds a lot better!

@juliandescottes
Copy link
Collaborator Author

Released in 0.4.0 .

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

No branches or pull requests

2 participants