#1238 Merge Shape and Path blocks

[ghost]

Not ready for merge.

Initial changes to merge path blocks into shape blocks. Started by creating blocks that allow arcs and bezier/quadratic curves to be drawn as stand alone shapes. Also added a polygon block.

Next I will be moving on to drawing a set of paths, similar to the current create set of paths block that will take all these shapes and draw them together, so there will likely be some changes to the current implementations for these blocks.

[CelticMinstrel]

Wait, adding a polygon block? And that other PR (#1241) also adds a polygon block? This seems like a bad idea...

[ghost]

@CelticMinstrel the other PR doesn't have anything for the polygon block. Like I said in that thread, the screen shot just happen to show the block that I was working on for this other issue. The two PR's have no overlapping code

[CelticMinstrel]

Ahhh, okay then. Sorry for posting without even looking at the code.

[kotarCreative]

@AdriennePind Can you post when this is ready for review?

[ghost]

Ready for review now.

I added a block to draw a set of paths and a last point and close path block to make drawing connected paths easy. One thing that is a little weird is the last point block. The CanvasRenderingContext2D library doesn't have an API for getting the location of the last point, or for determining if a path has already been started (at least not that I could find), so I have the lastPoint as a boolean for weather a moveTo operation should be done or not. It is working as it should with the path set. When lastPoint is used outside of a path it does not default to <0,0> as desired and for a few of the blocks it does not draw a shape.

[dethe]

Since we are the ones making all calls to the CanvasRenderingContext2D APIs, we could simply store the last point somewhere for later reference. It would be one more thing we need to clear when restarting the script though.

The intention of last point is to be a check for whether or not to call moveTo, the only reason for it to have default values is if it is used in a different context, since we don't have a good way to prevent that.

[ghost]

Yeah I thought storing it might be the solution for this. I will add that before our meeting tomorrow

[dethe]

Here's the gist ID of the script I'm exercising it with building up a single path: c9a6996d391a8453cce8 And here's a gist for drawing path components on their own: 1ca65d10050d03411f1e

[dethe]

Is there a reason I cannot add a circle, ellipse, rectangle, polygon, or set of paths to a set of paths? Isn't a shape also a path segment?

[ghost]

The path set right now only excepts paths (ie, lines, arcs, curves). It is possible to switch it to allow the other shapes to be added. I wasn't sure if that made sense, or was a behaviour we wanted, which is why I left the path symbol for those blocks to differentiate between the two.

[dethe]

re: Paths vs. Shapes

My preference is to go with whatever is more intuitive, easier to learn and easier to use for casual users. That said, my intuition about these things is not always right, since I'm not really the target audience. So I'm going to say what my feelings are on this, but keep in mind that I'm well aware my feelings could be totally wrong in this case and I'm willing to be convinced of that.

My feeling is that each block in the shapes menu that currently returns a path or a shape, should return a shape. Last point should work with any of them and be updated by any of them (we'll have to decide what that means in some cases). We should rename PathSet to be something like 'Compound Path' or just 'Shape' which groups a number of these shapes together into one complex shape.

Of course, we could group things in other ways, making each block a step and assembling them in a Context block, but I think it is worth keeping them somewhat separate this way. I don't know if we want to allow "path-effecting" blocks like lineWidth or lineColor to also be added to whatever we replace PathSet with, but my thinking is to not do that.

So those are my thoughts. Let me know what you think.

[CelticMinstrel]

Is there a reason I cannot add a circle, ellipse, rectangle, polygon, or set of paths to a set of paths? Isn't a shape also a path segment?

I don't think I'd call it a path segment. A shape can be thought of as a closed path. If a "set of paths" is literally just a set of disconnected paths, then it makes sense to add a shape to it, but if a "set of paths" is a set of instructions to draw an arbitrarily complex path (or shape), then I think it doesn't make much sense to add a shape to it.

I'm not 100% what "last point" is, but based on my interpretation of Adrienne's description, it would be hard to update it after drawing a closed shape. Obviously you'd want to set it to one of the vertices of the shape, but it's usually not obvious which vertex. Plus, some of them don't even have vertices.

[ghost]

To me it doesn't make a lot of sense to have the shapes be part of the paths set. I was initially going to allow a shape to be added to a path, and it is easy enough to change the implementation to do this, but as I was thinking about it, it just seemed pretty weird. I agree with @CelticMinstrel that it really depends on how we are interpreting what a path is. My default is to think of a path as a set of line segments or curves and I wouldn't consider a shape to fit into that criteria. Another reason why I thought keeping the distinction would be good is that I don't think the set of paths block would be intuitive if we added shapes to it, but that is likely because of my interpretation of a path. I guess if we were to change the block to be something like shape then it might clear up some of these issues.

[dethe]

I guess my idea about the shapes comes from Logo and Turtle Graphics generally, which don't make a distinction between paths and shapes (a shape would just be a sequence of paths, but is still a path). Also I'd really like to reduce the number of conceptual objects, so from that point of view merging paths and shapes would be a win in the same way as merging strings and text or merging points and vectors. For each of these, there's an argument to be made that they are different things, but from our user's point of view, do they really need to care?

I certainly agree that set of paths doesn't make sense as a name for the grouping that includes what are now both paths and shapes.

I'm curious, how are shapes not sets of line segments or curves?

[dethe]

As for some shapes not having vertexes, presumably that would be circles and ellipses? I think treating the centre of a circle as it's first and last point is how Python's turtle module deals with it.

Last point is a way of working around the CanvasAPI where each drawing command implicitly starts where the last command left off. If you want to make the paths disconnected you would insert a moveTo between commands, which moves the last point without drawing. Each series of drawing commands also begins with a moveTo to establish the point the command sequence should start from. This isn't a great match for our blocks, so we have the workaround of last point which in a sequence of drawing commands actually means "do nothing", but if the user puts a vector/point in its place would cause a moveTo to be insesrted. Since we have no way of controlling where the last point is used, we also need to handle cases where it is used outside of the context we're working around, so giving it a default value or actually storing the last point.

[ghost]

That does make sense. And probably renaming the block would make everything fine. Shapes definitely are a set of line segments and curves in themselves. I think what I meant was that they are a their own 'path' so added a shape to a path is kind of like containing a path in a path. I was just thinking more along the lines of a path containing standalone line segments/curves. I'm not opposed to having the shapes in a path as long as we rename the block. My main issue with it was that the block didn't seem intuitive with shapes in it and calling it a 'set of paths'.

[CelticMinstrel]

AdriennePind wrote:

My default is to think of a path as a set of line segments or curves and I wouldn't consider a shape to fit into that criteria.

Actually, a shape does fit that criteria.

I think what I meant was that they are a their own 'path' so added a shape to a path is kind of like containing a path in a path.

They are a closed path — is that what you mean here? In general, composing a path from several subpaths makes perfect sense, but when one of those subpaths is a closed path, it becomes more complicated.

dethe wrote:

As for some shapes not having vertexes, presumably that would be circles and ellipses? I think treating the centre of a circle as it's first and last point is how Python's turtle module deals with it.

I don't know anything about a Python turtle module, but in UCB logo you basically have to fake circles by drawing a regular polygon with 100 or more sides, and a similar method for ellipses. (Though it does also have an arc command which draws a circular arc centred on the turtle. Nothing equivalent for elliptical arcs or beziers, though.)

Treating the centre of a circle as its first and last point would work, but it's weird because this point is not on the perimeter of the circle. Furthermore, ellipses have two focal points rather than one, so the problem of which point is the reference point will still remain.

[dethe]

I'm OK with the circle's point not being on the perimeter (paths can have breaks in them anyway). Ellipse is problematic. Either we pick one focal point or the other, or one is starting point and one is end point, or we average them to get the true centre (problematic, but maybe the best option?).

[CelticMinstrel]

But then, which one is the start point and which one is the end point? Using the centre for an ellipse could work, I suppose.

Also, if paths can have breaks in them, then I wonder if you should reconsider the name "path".

[dethe]

I'm inclined to ditch the word "path" altogether and just called everything a "shape".

[CelticMinstrel]

I'm not sure that really makes sense either... like I mentioned earlier, a "shape" would normally be a closed path. (And if there can be breaks in the path, calling it a "shape" doesn't change anything either.)

[ghost]

I'm just about finished with some of the discussed changes but I still can't figure out how to calculate the end point of an arc. Anyone have any ideas how to do that? I need to calculate it so I can update the lastPoint after it is drawn

[CelticMinstrel]

Assuming you know the radius and angle of the arc, you should be able to use trigonometry on the resulting isosceles triangle to find the chord length. From there, it's trivial to get the position of the far endpoint of the chord (which is also the far endpoint of the arc itself).

[ghost]

The API we are using uses a start point, two control points and a radius to define the arc. I'm not clear on how the control points are used, the documentation isn't very clear

[CelticMinstrel]

From fiddling with the example on that page, it seems the interface is a bit weird. In particular, it appears to draw a line before initiating the arc in certain case.

The two control points appear to define a tangent to the curve, so if you insist on using arcTo, then the end point would be the point on the line for which there exists a circle touching the line. I believe the start point effectively creates a second tangent line, so from two tangent lines it should be possible to get the point you need with a little trigonometry.

I did however notice that there's an alternate arc-drawing command which might be a little easier to work with. Using that command, the method I described in my previous post should be possible, and I think the definition here is easier to understand (but, I don't know how others would feel about it). The angle for calculating the endpoint would be the difference between the two passed angles. The radius is the Euclidean distance between the start point and the passed centre point.

...of course, there's also the option of providing blocks for both methods of defining a circular arc...

[ghost]

Dethe and I talked about it and he agreed that the arcTo() function didn't make sense so we decided to switch to the arc() one instead

[dethe]

I think we should go ahead and merge this and I'll fix the issue where blocks are called twice. I've figured it out, but need to fix in my branch and then merge. :shipit:

[ghost]

Okay. I have one more change to push so the shapes can be added to a path. I am just about done it so I will do that soon.

[dethe]

Cool. I think there are some conflicts between your branch and mine, so once yours is merged I'll resolve the conflicts before merging mine.

[ghost]

@dethe I have pushed the last of my changes if you want to take a look before I merge the branch. I changed the "set of paths" block to just "path" that takes any shape as input. There are also 3 options for the path; connected, connected and closed, disconnected. These were necessary because of how the library deals with a path. When it is connected I cannot do a moveTo() or beginPath() or else the context of what the current path is does not stay consistent, so if the point for the next shape is not lastPoint but the path is connected, there will be a line connecting the endpoint of the last shape to the start point of the next shape. Here's a gist of what I was playing around with to test this, 2400b57a18f94cbe872c. I think it works well and at least is clear on what the options are for how a "path" can be drawn. We may still want to change the name of the block to something else, especially since it doesn't really make sense to have a disconnected path, but I think being able to draw a set of disconnected shapes is good, I just didn't have any ideas for a better name yet.

[dethe]

@AdriennePind This has been an epic one! It looks good, go ahead and merge it in. Any remaining changes can be a separate PR after we've had time to play around with this for awhile. In the meantime there are some other PRs to merge with this work. :shipit:

[dethe]

Actually, I'm going to go ahead and pull this in so I can merge my changes on top of it.