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

Perspective mode #685

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Perspective mode #685

wants to merge 4 commits into from

Conversation

gwojcik
Copy link
Collaborator

@gwojcik gwojcik commented May 22, 2016

First part of changes for perspective mode.

Description of changes:
After first activation default vanishing points are created. They are placed
around point (0, 0) on canvas.
Vanishing point details can be viewed in "Tool Options".
"Right" and "Left" points define horizon. Dragging these point rotate
all points around "Center" point.
"Center" point can be dragged to change position of all points.
"Horizontal" and "Vertical" allow drawing horizontal and vertical lines,
they are influenced by rotation of "Right" and "Left" points.

Perspective Tool Options
"Add" - add point after clicking on canvas
"Delete" - delete pint selected on list
"Edit" - edit points on list, works only for added points
"Set active" - active points are visible on canvas and lines can be draw
to them. Active points have open eye icon on list.

Clicking on canvas start drawing of line to one of vanishing points.
Clicking with shift start drawing line starting in position of point
selected on list to current mouse position.

There are still need for some changes.

  • better use interface
  • saving perspective data
  • groups of perspective points, allowing transformation of all pints in group
  • other

I don't think this changes are reedy for merge with master.
What should be changed next?
Maybe on community forum should be created discussion about perspective mode?

@achadwick achadwick added the type.Enhancement Issue requests feature. label May 22, 2016
@achadwick achadwick added this to the MyPaint v1.3.0 milestone May 27, 2016
@achadwick
Copy link
Member

This is a really interesting start, and I want to pick it up for 1.3.0.

I really love how you lay down strokes towards the control points with the brush, and it just does the right thing. This has a deeply satisfying feeling to me, and it's exactly the sort of intuitive thing MyPaint tools should have.

Can you start by rebasing against the latest mypaint/master? There has been a fairly major project split, and there are some changes that this branch needs to incorporate in order to build against the upcoming libmypaint release. A forced push will be OK.

Some quick UI notes:

  • The options panel needs a lot of work before it can be merged. Buttons enabling and disabling in response to list choices, that sort of thing.
  • It seems very unclear how new vanishing points become viewing-plane guides without a visible point on the canvas.
  • Can you explain the angle logic a bit more?
  • Does this system rely on magic control point names? These will be translated, and it will be hard for users to remember what they are.
  • Sometimes the contents of the edit box disappear, I think after the window X button is pressed. Perhaps the "destroy" handler should hide the contents instead of its default behaviour? It can cause crashes.

image

  • It would be good if the tool respected pen pressure. People will like to sketch perspective guides lightly.
  • Would future integration with straight line and ellipse modes work?

@odysseywestra
Copy link
Member

odysseywestra commented May 29, 2016

I've put my thoughts down about the perspective mode on the thread which @achadwick started on our community forums:

https://community.mypaint.org/t/testers-wanted-perspective-mode-early-stages/369/7?u=odysseywestra

It's a promising start, but I can't wait to see it implemented into MyPaint. Great job @gwojcik!

@gwojcik gwojcik force-pushed the perspectiveMode branch 2 times, most recently from 5297e09 to 274586b Compare May 30, 2016 18:52
@gwojcik
Copy link
Collaborator Author

gwojcik commented May 30, 2016

Branch is now rebased against mypaint/master.

It seems very unclear how new vanishing points become viewing-plane guides without a visible point on the canvas.

I don't understand what you mean. Points should appear after using 'Add' button and clicking on canvas.

Can you explain the angle logic a bit more?

I'll describe it on community forum.

Does this system rely on magic control point names? These will be translated, and it will be hard for users to remember what they are.

Names are not important. Currently only default points have special properties (scaling/rotation and moving other points).

Sometimes the contents of the edit box disappear, I think after the window X button is pressed. Perhaps the "destroy" handler should hide the contents instead of its default behaviour? It can cause crashes.

I'll check it.

It would be good if the tool respected pen pressure. People will like to sketch perspective guides lightly.

Currently it is possible to change perspective point while drawing line. Pressure data should be stored while drawing and after changing vanishing point interpolated and used in new line for that point. Now perspective line is drawing use same function as line mode. Making drawing incremental also would be good thing.

Would future integration with straight line and ellipse modes work?

What do you mean by integration?

@odysseywestra
Copy link
Member

@gwojcik could you rebase your PR to the latest commit from master?

@gwojcik gwojcik force-pushed the perspectiveMode branch 2 times, most recently from 138ad11 to dee3ef1 Compare April 7, 2017 21:04
@achadwick
Copy link
Member

Okay, testing this out. I'm liking the options panel a lot more now, although I think there are a lot of rough edges still.

I managed to trigger a new exception :/

  1. Launch a fresh MyPaint with python setup.py demo
  2. Delete the stock Configuration group
  3. Delete and add several more points
  4. Click on 3P
  5. Click on the canvas
Mypaint version: 1.3.0-alpha+git.1069fdf3
System information: Linux-4.9.0-2-amd64-x86_64-with-debian-9.0
Using: Python 2.7.13, GTK 3.22.11, GdkPixbuf 2.36.5, Cairo 1.14.8, GLib 2.50.3
Traceback (most recent call last):
  File "/tmp/demo-pxCK_Q/lib/mypaint/gui/tileddrawwidget.py", line 1159, _draw_cb(self=<tileddrawwidget.CanvasRenderer object at 0x7f2a...ileddrawwidget+CanvasRenderer at 0x5584d99290f0)>, widget=<tileddrawwidget.CanvasRenderer object at 0x7f2a...ileddrawwidget+CanvasRenderer at 0x5584d99290f0)>, cr=<cairo.Context object>)
                cr.save()
                overlay.paint(cr)
                cr.restore()
  variables: {'cr': ('local', <cairo.Context object at 0x7f2ac423ea10>), 'overlay.paint': ('local', <bound method Overlay.paint of <gui.perspective.Overlay object at 0x7f2add8766d0>>)}
  File "/tmp/demo-pxCK_Q/lib/mypaint/gui/perspective.py", line 598, paint(self=<gui.perspective.Overlay object>, cr=<cairo.Context object>)
                point = self._perspective.get_point_by_uid(uid)
                text = point.name
                layout = self._tdw.create_pango_layout(text)
  variables: {'text': (None, []), 'point': ('local', None)}
AttributeError: 'NoneType' object has no attribute 'name'

@gwojcik
Copy link
Collaborator Author

gwojcik commented Jun 3, 2017

@achadwick I was not able to reproduce this exception, but I think, I managed to fix it in last commit.
Probably variable storing last used point id was pointing to deleted vanishing point.

@achadwick
Copy link
Member

Some more thoughts on this in its current form:

Showstopper: it's possible to delete everything from the configuration panel. After this, I cannot click on any of the add/delete/2P/3P buttons to start again.

Showstopper: There are some major problems with the circle/triangle/square geometry features:

  1. Start with the default config
  2. Click on square
  3. Click to start a drag on the canvas, below the 3 dots
  4. Drag upwards, above the 1st dot. The perspective square now draws as an arrowhead

image

  1. Keep dragging, but go over to the right a bit. It's now a bow-tie...

image

  1. Even odder things happen with the circle button: sometimes, weird hyperboloid oddities occur, like this:

image

If the perspective mode feature is to be usable, it should prevent impossible shapes from being drawn, and show some feedback more generally. Would it be possible to overlay some construction lines for things to make it clearer what clicking and dragging would draw (or not?)

@achadwick
Copy link
Member

I would like the perspective settings and vanishing points to be saved as part of the working document. I am working on a patch to allow a doc.settings object with full observe-ability bells and whistles, so watch this space 😄

@gwojcik
Copy link
Collaborator Author

gwojcik commented Sep 5, 2017

Thanks for information and testing.

Showstopper: it's possible to delete everything from the configuration panel. After this, I cannot click on any of the add/delete/2P/3P buttons to start again

Add button should be always accessible, this bug was added recently with code for enabling and disabling buttons.
Adding reset function or presets will also be good thing.

There are some major problems with the circle/triangle/square geometry features

I know about them. I should prevent drawing shapes after moving cursor to other side of "vanishing horizon".

If the perspective mode feature is to be usable, it should prevent impossible shapes from being drawn, and show some feedback more generally. Would it be possible to overlay some construction lines for things to make it clearer what clicking and dragging would draw (or not?)

I agree that adding overlay with lines will be helpful. There could be some performance problems with overlays? I tested overlays some time ago on older computer and performance was not good enough for interactive part of interface.

I would like the perspective settings and vanishing points to be saved as part of the working document. I am working on a patch to allow a doc.settings object with full observe-ability bells and whistles, so watch this space 😄

Will this require xml data format? I will be looking for this change.

@odysseywestra
Copy link
Member

Tiage Note: Moving this to limbo for now, and make this the next big feature for us when it comes to the next version of MyPaint.

'lib/libperspective/Projection.cpp',
],
swig_opts = ['-Wall', '-noproxydel', '-c++', '-outdir', 'lib'],
language = 'c++',

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unexpected spaces around keyword / parameter equals

'lib/libperspective/PythonGraph.cpp',
'lib/libperspective/Projection.cpp',
],
swig_opts = ['-Wall', '-noproxydel', '-c++', '-outdir', 'lib'],

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unexpected spaces around keyword / parameter equals

""" update data returned from get_default_data
insert additional_params into node with id == node_id
"""
for node in data['nodes']:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Black would make changes.

points_3d = []
for point in points_cube:
point_3d = corner_3d + p.rotate(space.get_rotation(),
point.scalar_mul(self._cube_size)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

continuation line under-indented for visual indent

view = self._space.get_view().as_projection()
center = view.get_center_complex()

corner = center + self._cube_corner * view.get_rotation() * view.get_size()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line too long (83 > 79 characters)

center = view.get_center_complex()
up = p.rotate(space.get_rotation(), Quaternion(0, 1, 0, 0))

corner = center + self._cube_corner * view.get_rotation() * view.get_size()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line too long (87 > 79 characters)


for index, point in enumerate(points_cube):
point_3d = corner_3d + p.rotate(space.get_rotation(),
point.scalar_mul(self._cube_size)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

continuation line under-indented for visual indent

view = self._space.get_view().as_projection()
center = view.get_center_complex()

corner = center + self._cube_corner * view.get_rotation() * view.get_size()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line too long (83 > 79 characters)

self._queue_draw_node(point_pos)

def get_overlay_widgets(self):
default_color = gui.style.EDITABLE_ITEM_COLOR

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

local variable 'default_color' is assigned to but never used

import gui.dialogs

from gui.overlays import rounded_box
from gui.perspectivedata import (get_default_data, update_data)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Black would make changes.

@jplloyd
Copy link
Member

jplloyd commented Sep 23, 2020

Nice to see this get picked up again! The travis build image might need to be bumped to bionic if some dependencies are too old.

As for the hound warnings, when the PR is otherwise ready, you can ignore the "Black would make changes" complaints. The rest should be fixed, however.

changes:
- remove `#include <variant>`, std::variant is part of c++17
- Add missing inline. Remove conditional constexpr for recent version of gcc, it was used
  for functions with math functions calls. Math functions are constexpr
  only in g++, clang++ does not allow it.
data = {
"nodes": [
{"type": "Group", "id": "Root", "name": _("Root")},
{

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Black would make changes.

@gwojcik
Copy link
Collaborator Author

gwojcik commented Sep 25, 2020

Most problems reported by hound was fixed, only left are "Black would make changes" and E251 - "unexpected spaces around keyword / parameter equals". Second problem is ignored in setup.cfg. I saw commits in which you tried to configure hound to use configuration from file, but there were some problems.
Most important thing left to do is good user interface.

@mypaint mypaint deleted a comment from houndci-bot Oct 8, 2020
@mypaint mypaint deleted a comment from houndci-bot Oct 8, 2020
@mypaint mypaint deleted a comment from houndci-bot Oct 8, 2020
@mypaint mypaint deleted a comment from houndci-bot Oct 8, 2020
@mypaint mypaint deleted a comment from houndci-bot Oct 8, 2020
@mypaint mypaint deleted a comment from houndci-bot Oct 8, 2020
@mypaint mypaint deleted a comment from houndci-bot Oct 8, 2020
@mypaint mypaint deleted a comment from houndci-bot Oct 8, 2020
@mypaint mypaint deleted a comment from houndci-bot Oct 8, 2020
@mypaint mypaint deleted a comment from houndci-bot Oct 8, 2020
@mypaint mypaint deleted a comment from houndci-bot Oct 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type.Enhancement Issue requests feature.
Development

Successfully merging this pull request may close these issues.

None yet

5 participants