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

add arrows and arrows3d renderers #67

Merged
merged 20 commits into from
Sep 1, 2020
Merged

add arrows and arrows3d renderers #67

merged 20 commits into from
Sep 1, 2020

Conversation

bdeket
Copy link
Contributor

@bdeket bdeket commented Aug 26, 2020

PR to address remaining issues of #42

some points:

  • I decided to stay closer to the line implementation, so the (Sequenceof (Sequenceof number)) are displayed as head/tail connected arrows. In my use case, I almost never wanted to plot more than one vector per call (so I could set color and label per vector). If more vectors in the same category need to be shown with gaps between head/tail of 2 consecutive arrows, this can be achieved as shown in the documentation.
  • I made the parameter for arrow-head-angle absolute
  • and the parameter for arrow-head-size-scale possibly both. The absolute version is crippled by the fact that at this point we don't know the actual size of the vector (only the pixel size). Then again, this has it's good points if you start zooming interactively.

Sample code:

#lang racket
(require plot)
(arrow-head-size-or-scale '(= 25))
(arrow-head-angle (* 1/20 3.14))
(plot (list (arrows '((1 1) (3 2) (4 0.5)) #:width 3 #:color 1)
            (arrows '((1 1) (4 0.5)) #:width 3 #:color 2))
      #:x-min 0.5 #:x-max 4.5
      #:y-min 0 #:y-max 2.5)

Produces:

arrows

pbpf and others added 7 commits March 7, 2018 18:09
add arrow-head-size-scale & arrow-head-angle-scale to allow user change behavior of function draw-arrow.
add two parametersto function draw-arrow
now it is (draw-arrow v1 v2 [head-size-scale (arrow-head-size-scale)][head-angle-scale(arrow-head-size-scale)])
add arrows and arrows3d to plot arrows
@alex-hhh alex-hhh changed the title Arrows add arrows and arrows3d renderers Aug 27, 2020
Copy link
Collaborator

@alex-hhh alex-hhh left a comment

Choose a reason for hiding this comment

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

I added some comments about his changes, but it is getting late here and I will finish up the review tomorrow. In particular I would like to propose some changes to the documentation wording of the new parameters and renderers.

Overall the changes look good, but there are two things I find odd, although they won't prevent merging this pull request:

  • the fact that arrows are connected to each other seems un-intuitive to me, I would have expected each arrow with start and end points to be specified individually. Also, I would have expected to be able specify the arrow as a position + direction (with magnitude) vector, in addition to the start and end positions
  • the fact that the arrow head parameters cannot be specified as function parameters for the renderer seems a limitation to me. It is OK to have parameters at the "plot level", but other drawing attributes of the arrows (e.g. line style, width and color) can be set for each individual renderer.

plot-doc/plot/scribblings/params.scrbl Outdated Show resolved Hide resolved
plot-doc/plot/scribblings/renderer2d.scrbl Outdated Show resolved Hide resolved
plot-lib/plot/private/plot3d/line.rkt Outdated Show resolved Hide resolved
plot-lib/plot/private/common/parameters.rkt Show resolved Hide resolved
plot-test/plot/tests/PRs/42.rkt Outdated Show resolved Hide resolved
plot-doc/plot/scribblings/renderer2d.scrbl Outdated Show resolved Hide resolved
plot-lib/plot/private/plot2d/line.rkt Outdated Show resolved Hide resolved
@bdeket
Copy link
Contributor Author

bdeket commented Aug 27, 2020

I think you're comment about wanting to specify the arrow by base-point and vector is valid. If it's ok I will also move the renderers to their own files.

@bdeket
Copy link
Contributor Author

bdeket commented Aug 27, 2020

Tomorrow I will try to expose the arrow-head parameters to the renderer. And add more tests.
Should I also do so for the vector-field? With the current implementation, that is also affected.
Should I rebase on master to resolve the conflict?

Copy link
Collaborator

@alex-hhh alex-hhh left a comment

Choose a reason for hiding this comment

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

I added some more comments based on your last updates, but overall the changes look good. At this time, it seems you are the only one which will use the arrows renderer and if this interface suits you, we can stop here. We can always extend the interface later to accept the size and head as parameters to the renderer.

With regards to merging or rebasing

I will squash merge this pull request, so it does not matter to me. I personally prefer merging, as it seems simpler to me and there is less chance of loosing changes, but it is your repository, and your choice :-)

With regards to adding tests

This pull request should add tests which cover the basic functionality, but we don't need to add too many individual tests. I think the following would be sufficient:

  • one 2D plot of "connected arrows" and "point +_magnitude" arrows (on the same plot), with a custom arrow size using absolute values and a custom arrow angle
  • one 3D plot of "connected arrows" and "point + magnitude" arrows (on the same plot), with a custom arrow size using relative values and a custom arrow angle
  • one 2D vector field plot with a custom arrow size using absolute values and a custom arrow angle
  • one 3D vector field plot with a custom arrow size using relative values and a custom arrow angle

for the vector field plots, you can copy some existing vector field examples -- however, add the new tests to the test file of this pull request -- one way to be confident that this code does not break existing plots is that the existing tests don't change...

plot-doc/plot/scribblings/renderer3d.scrbl Outdated Show resolved Hide resolved
plot-doc/plot/scribblings/params.scrbl Outdated Show resolved Hide resolved
plot-doc/plot/scribblings/renderer2d.scrbl Outdated Show resolved Hide resolved
plot-doc/plot/scribblings/renderer2d.scrbl Outdated Show resolved Hide resolved
@bdeket
Copy link
Contributor Author

bdeket commented Aug 28, 2020

I think this fixes the remaining points. Thanks already for putting your time into reviewing this.

@alex-hhh
Copy link
Collaborator

Thanks for removing the mention of the "nan". As you know, the feature will remain in the code base and it is unlikely to change.

Also, I actually have a use case which draws a lot of disconnected lines as separate lines renderers -- this is somewhat slow as there are thousands of lines. I will try the "nan" trick to see if the rendering is faster in that case and if it turns out that using this technique improves rendering performance, I will update the documentation and add some unit tests to make it official.

I will give this PR another look tomorrow morning (Australia time...) before merging it in, but it looks good to me. Thanks for making all the changes and picking up the work that was started two years ago.

@alex-hhh
Copy link
Collaborator

Unfortunately, we have a bug when zooming, unzooming the plot:

arrow-problems

This is probably caused by the fact that the plot-area draw-arrow method will not draw an arrow if its base is not in view, but given that this is used for vector fields as well, I don't want to immediately remove it, although probably extending the check to include whether v2 is in bounds would help. Will need to see what the implications are of changing it, and check if there is a similar problem for 3d plots (although they cannot be zoomed in)

(when (and v1 v2 (in-bounds? v1))

@alex-hhh
Copy link
Collaborator

alex-hhh commented Aug 29, 2020

I added a check to render an arrow if either end is in the plot area, but now it fails if both ends are outside, but the body is inside the plot -- I think the main issue for this is that the original arrow code assumed that arrows would be small, since they were used for vector fields. Now with these renderers arrows can be quite long.

I don't want to remove the end-point check unconditionally, as I am not sure what effect would have on the performance of the vector field renderers.

The 3d plot seems to handle this case differently, but it would have problems with the arrow head: if both ends are outside of the plot, it uses put-line. However, given that arrow heads are now configurable, it is possible that parts of the arrow head would be visible even if the actual tip of the arrow is not.

arrow-problems1

@alex-hhh
Copy link
Collaborator

My only idea now is to have another method on plot-area%, perhaps called put-arrow/no-culling which does not check the bounds and always draws the arrow (the area clipping will make sure only the visible parts are visible). This method would be used by the arrows renderer, while put-arrow would be used by vector field. We'll need some comments in the code of why the two methods exist.

I don't want to add a "clip" flag to put-arrow as I am concerned about performance problems since vector-field draws a lot of arrows....

A similar thing needs to be done for the 3D plot.

Do you have any other idea about this?

@alex-hhh
Copy link
Collaborator

alex-hhh commented Aug 29, 2020

I reverted my last change, as I think we should leave put-arrow as is. Here is an example that can be used to get this working -- note that this defect is not related to zooming or unzooming the plot, the defect exists when arrows are outside the plot area.

Four arrows should be visible in each plot (the relevant part of them). Once this is fixed, the code below can be converted to a unit test, to make sure the functionality will not be broken in the future.

#lang racket
(require plot)
(arrow-head-size-or-scale '(= 50))
(plot (list
       (arrows '((0 5) (3 5)) #:color 1 #:width 3 #:label "start OUT, end OUT")
       (arrows '((0 4) (2 4)) #:color 2 #:width 3 #:label "start OUT, end IN")
       (arrows '((1.5 3) (3 3)) #:color 3 #:width 3 #:label "start IN, end OUT")
       ;; Note: the two ends of the arrow head should be visible inside the plot
       (arrows '((1.5 2) (2.6 2)) #:color 4 #:width 3 #:label "start IN, end JUST OUT (ears visible)")
       (arrows '((0 1) (2.6 1)) #:color 5 #:width 3 #:label "start OUT, end JUST OUT (ears visible)"))
      #:x-min 1 #:x-max 2.5
      #:y-min 0 #:y-max 6
      #:legend-anchor 'bottom-left)
(plot3d (list
       (arrows3d '((0 5 0) (3 5 0)) #:color 1 #:width 3 #:label "start OUT, end OUT")
       (arrows3d '((0 4 0) (2 4 0)) #:color 2 #:width 3 #:label "start OUT, end IN")
       (arrows3d '((1.5 3 0) (3 3 0)) #:color 3 #:width 3 #:label "start IN, end OUT")
       ;; Note: the two ends of the arrow head should be visible inside the plot
       (arrows3d '((1.5 2 0) (2.6 2 0)) #:color 4 #:width 3 #:label "start IN, end JUST OUT (ears visible)")
       (arrows3d '((0 1 0) (2.6 1 0)) #:color 5 #:width 3 #:label "start OUT, end JUST OUT (ears visible)")
       )
      #:x-min 1 #:x-max 2.5
      #:y-min 0 #:y-max 6
      #:z-min -1 #:z-max 1
      #:legend-anchor 'bottom-left)

@bdeket
Copy link
Contributor Author

bdeket commented Aug 29, 2020

Cool.
I think we can do the 'end' checking in the render function. And use put-lines instead of put-arrows if the beginning and end are outside of the area bounds. I will look deeper into this on Monday.

@bdeket
Copy link
Contributor Author

bdeket commented Aug 29, 2020

Thinking a bit more about this, I want to check how often vector-field skips a vector. If I remember correctly, it divides the given area in squares and uses it's center point to draw an arrow. If the arrows are automatically scaled, the vectors will always be inside the plotting area. If a different scaling is used. I think an argument can be made for leaving the base of the vector drawn anyway, even if the end is not.
At least for vector-field, I think one end will always be in the plot. So it's clear which way the arrow is pointing.
For arrows on the other hand, if you zoom in on the middle, the direction is not clear anymore...

@bdeket
Copy link
Contributor Author

bdeket commented Aug 31, 2020

I decided not to draw the arrow-head when the end-point is not in the plot. It gets very unpredictable/counterintuitive when you start zooming. If the size is absolutely defined, zooming in just behind the end might make the arrow-head disappear (because in pixels, the end is now a long way away). Similarly, the angle is defined in absolute dc-coordinates, if you zoom in with a wide y range and a short x range, the end of the arrow can jump around in unexpected ways.
The only real alternative I see is to draw the arrows in plot-coordinates.

Copy link
Collaborator

@alex-hhh alex-hhh left a comment

Choose a reason for hiding this comment

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

I did not have time to review the changes in detail (will do so over the next few days, but here is some initial feedback)

There are two mistakes in the documentation (I pointed them out in comments)

I would like the test cases to be simplified and based on the example given in one of my previous comments. The problem is that, when tests fail, the developer has to compare the sample image with the new image to determine what is wrong, and this can be sometimes difficult if the plots show a seemingly random assortment of arrows. As such it would be good if the sample plots would show clear and easy to follow examples of for both the 2d and 3d plots (each case should have a different color and a short label identifying it in the legend):

  • an arrow drawn completely inside the plot area
  • an arrow drawn with the start outside the plot area but the end inside
  • an arrow drawn with the start inside and the end outside ("just outside" can also be made a test case, to show that we don't draw the arrow head)
  • an arrow drawn with the start and end outside
  • the above can be mixtures of point-to-point arrows and "origin + magnitude" arrows as well as different combinations of the head size and angle specifications.
  • perhaps add an example of "chaining of arrows", i.e. drawing multiple arrows using a single renderer.

The example I gave already covers most of these cases.

@@ -358,24 +358,27 @@ For example, to plot an estimated density of the triangle distribution:
[#:width width (>=/c 0) (arrows-line-width)]
[#:style style plot-pen-style/c (arrows-line-style)]
[#:alpha alpha (real-in 0 1) (arrows-alpha)]
[#:arrow-head-size-or-scale size (or/c (list/c '= (>=/c 0)) (>=/c 0)) (arrow-head-size-or-scale)]
[#:arrow-head-size-or-scale angle (>=/c 0) (arrow-head-angle)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would presumably be arrow-head-angle?

@@ -142,6 +142,8 @@ Returns a renderer that plots a vector-valued function of time. For example,
[#:width width (>=/c 0) (arrows-line-width)]
[#:style style plot-pen-style/c (arrows-line-style)]
[#:alpha alpha (real-in 0 1) (arrows-alpha)]
[#:arrow-head-size-or-scale size (or/c (list/c '= (>=/c 0)) (>=/c 0)) (arrow-head-size-or-scale)]
[#:arrow-head-size-or-scale angle (>=/c 0) (arrow-head-angle)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here.

Copy link
Collaborator

@alex-hhh alex-hhh left a comment

Choose a reason for hiding this comment

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

Thanks for doing the latest round of changes -- I think adding the arrow renderers was more work than you initially estimated...

There are a few more issues with the arrows renderers, but they are not critical and I think this pull request is good to merge. I will document the remaining problems in a separate issue, and we'll work on them as time allows.

@alex-hhh alex-hhh merged commit 4415b2d into racket:master Sep 1, 2020
@bdeket bdeket deleted the arrows branch September 14, 2020 10:44
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.

3 participants