Skip to content

Draggable line #9

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.

Sign up for GitHub

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

Open
tfadali wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Draggable line #9

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d95abc9
start working on more draggable demos
tfadali Jul 20, 2023
25674ff
draggable line demo
tfadali Jul 20, 2023
84b34b4
Update draggable-line-demo.js with naming fix
GregStanton Jul 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions demos/draggable-line-demo.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
let w; //graph window
let myLine;
let draggable;

function setup() {
createCanvas(400, 400);

const xOrigin = width / 2;
const yOrigin = height / 2;
const xScale = 10; //px per unit
const yScale = 10; //px per unit
w = createGraphWindow(xOrigin, yOrigin, xScale, yScale);

myLine = w.createLine(-1, -2, 3, 4);
myLine.setIsDraggable(true)
Copy link
Copy Markdown
Contributor

@GregStanton GregStanton Jul 21, 2023

Choose a reason for hiding this comment

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

Let's come to a tentative consensus on the 'setIsDraggable' name before we merge this, since it affects the interface. Here are some issues to consider.

Comparison to our naming scheme for event listeners: The name is consistent with the naming scheme for event listener setters, such as setObjectIsHovered(). However, "setIsDraggable" sets a Boolean, whereas "setObjectIsHovered" sets a listener that returns a Boolean. This may be okay, since there are two cues that these setters accept different kinds of input. The biggest cue is the "-able" and "-ed" endings. The other cue is that the passive "isHovered" is paired with "Object" (the thing being hovered). So, the "setIsDraggable" name seems okay from this perspective.

Comparison to p5
Some cases where p5 uses "set" are as follows:

  • setAttributes() is a general setter for WebGL that can be called like setAttributes(key, value) or setAttributes(obj)
  • setMoveThreshold()
  • setShakeThreshold()
  • set() for setting pixels or writing a whole image to the display window
  • setCamera()
  • .set() for p5 vectors and for p5.Camera objects (p5.Camera objects also have .setPosition())

Some cases where p5 uses "is" in a function name: isLooping() and keyIsDown(). Neither is a setter. Both return Booleans, as we'd expect based on the usual convention.

Since the use of "setIsDraggable" seems consistent with usage in p5, and the use of "isDraggable" as a setter seems inconsistent with p5, the name "setIsDraggable" seems preferable. From this perspective, the current name seems good.

Comparison with the combined getter/setter approach of Konva.js: In this approach, methods like .stroke() do two things: they set the relevant property based on the arguments that are supplied, and they also return the value of the property. So, for example, calling .stroke() without arguments just gets the object's current stroke. In the same way, Konva.js has a .draggable() method that acts as a getter and setter for a Boolean "draggable" flag. However, "isDraggable" makes it more clear that we're dealing with a Boolean (due to the naming convention for Booleans), and .isDraggable() probably shouldn't be used as a setter, to avoid conflict with p5. Again, "setIsDraggable" seems best.

Tentative conclusion: The current name .setIsDraggable() seems to work best. For other functions like .stroke(), we can have a separate discussion to break down the pros and cons of combined get/set functionality. As with many things related to getters and setters, there's debate about whether combined getters/setters are generally helpful or unhelpful. Some pros and cons are discussed here:

Copy link
Copy Markdown
Contributor Author

@tfadali tfadali Jul 21, 2023

Choose a reason for hiding this comment

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

I think what's missing from this analysis is that setIsDrabble, to me, suggests that there is a boolean inside that we are setting. Other developers familiar with common naming conventions will also think that. It's true that you can look at the signature or docs and see that it's a callback, but that's elevating the importance of documentation, relative to making the API intuitive.

Copy link
Copy Markdown
Contributor Author

@tfadali tfadali Jul 21, 2023

Choose a reason for hiding this comment

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

Personally, I didn't pick up on the cues, I don't think. The naming convention is too ingrained for me.

Copy link
Copy Markdown
Contributor Author

@tfadali tfadali Jul 21, 2023

Choose a reason for hiding this comment

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

I don't really see the "-ed" or "-abble" as cues the way you do, and at least in the p5 editor, there isn't going to be any autocomplete suggestion that it takes a callback.


draggable = w.createDraggable();
draggable.mouseDragged(object => {
Copy link
Copy Markdown
Contributor

@GregStanton GregStanton Jul 21, 2023

Choose a reason for hiding this comment

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

In the design I have in mind, the default .mouseDragged() handler would work, so it wouldn't be necessary to set it in the sketch code. It could be customized, but for a sketch like this, it wouldn't be necessary. So, lines 18 through 25 involving mouseDragged() and mouseDropped() wouldn't be needed.

Were you planning to take out these lines once you implement more of the design? If not, I could explain the approach I'm thinking of. It's important that we make the basic interactions work as easily as possible. Of course, I'm not sure yet if the idea I'm thinking of will work quite as I hope!

Naming note: mouseDropped() is a placeholder name unless we can't find something better. Until now, it hasn't been a priority since we haven't needed to use it in the demo sketches.

Copy link
Copy Markdown
Contributor Author

@tfadali tfadali Jul 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Were you planning to take out these lines once you implement more of the design?

You guessed it.

const mouse = createVector(mouseX, mouseY);
const midpoint = object.getCenterInCanvas()
const diff = p5.Vector.sub(mouse, midpoint)
object.translate(diff)
});
draggable.mouseDropped(() => {})
}

function draw() {
background(240, 240, 240);
myLine.takeInput(draggable)
w.line(myLine);
}
2 changes: 1 addition & 1 deletion index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

<body>
<script src = "mathemagical-prototype.js"></script>
<script src="demos/interaction-demo.js"></script>
<script src="demos/draggable-line-demo.js"></script>
</body>
</html>

89 changes: 89 additions & 0 deletions mathemagical-prototype.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,15 @@ GraphWindow.prototype.point = function(...args) {
this.render('createPoint', ...args);
}

//Line
GraphWindow.prototype.createLine = function(x1, y1, x2, y2) {
return new Line(this, x1, y1, x2, y2)
}

GraphWindow.prototype.line = function(...args) {
this.render('createLine', ...args);
}

//Square
GraphWindow.prototype.createSquare = function(x, y, s) {
return new Square(this, x, y, s);
Expand Down Expand Up @@ -404,6 +413,76 @@ class Square {
}
}

// TODO: refactor to use Vertex objects instead of p5 vectors
class Line {
constructor(graphWindow, x1, y1, x2, y2) {
this.graphWindow = graphWindow;
this.verticesInGraph = [
createVector(x1, y1),
createVector(x2, y2)
]
this.verticesInCanvas = this.getVerticiesInCanvas();
this.strokeWeight = graphWindow.getStrokeWeight();
this.isDraggable = false;
}

setIsDraggable(isDraggable) {
this.isDraggable = isDraggable;
}

getVerticiesInCanvas() {
return this.verticesInGraph.map(
v => createVector(this.graphWindow.X(v.x), this.graphWindow.Y(v.y))
);
}

computeGraphFromCanvas() {
Copy link
Copy Markdown
Contributor

@GregStanton GregStanton Jul 21, 2023

Choose a reason for hiding this comment

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

The map() method is more concise than the loop I used for the same method in the Square class. There are some trade-offs, but if one implementation is clearly preferable on balance, then could you go ahead and make the corresponding methods in these classes consistent? Once we get to the point where we have a third repetition of these methods, we may want to go ahead and abstract them into a base class, but for now, we could skip that.

Copy link
Copy Markdown
Contributor Author

@tfadali tfadali Jul 21, 2023

Choose a reason for hiding this comment

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

I don't know if we have to go with 100% loops or 100% functional-style .map() stuff. For example, at my last job, we had the convention of using loops whenever the internal code mutated some state, and preferred functional-style for most other cases. Sometimes, for more complicated cases, a loop will be much easier to deal with.

Copy link
Copy Markdown
Contributor Author

@tfadali tfadali Jul 21, 2023

Choose a reason for hiding this comment

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

But yeah, we can make it consistent between methods that do the same thing and/or combine them into common abstract methods.

return this.verticesInCanvas.map(
v => createVector(this.graphWindow.x(v.x), this.graphWindow.y(v.y))
)
}

getCenterInCanvas() {
const xCenter = (this.verticesInCanvas[0].x + this.verticesInCanvas[1].x)/2
const yCenter = (this.verticesInCanvas[0].y + this.verticesInCanvas[1].y)/2
return createVector(xCenter, yCenter)
}

getWidthInCanvas() {
Copy link
Copy Markdown
Contributor

@GregStanton GregStanton Jul 21, 2023

Choose a reason for hiding this comment

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

I guess you hard coded 5 temporarily?

Copy link
Copy Markdown
Contributor Author

@tfadali tfadali Jul 21, 2023

Choose a reason for hiding this comment

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

Yeah, just getting it working for now.

return this.graphWindow.xScale * 5
}

getHeightInCanvas() {
return this.graphWindow.yScale * 5
}

takeInput(draggable) {
if (this.isDraggable) {
draggable.giveInput(this);
}
// TODO: each vertex needs to take input
}

translate(vector) {
for (let v of this.verticesInCanvas) {
v.add(vector)
}

//reset vertices in graph window coordinates
this.verticesInGraph = this.computeGraphFromCanvas(this.verticesInCanvas);
}

render() {
push();
const [start, end] = this.verticesInCanvas;
strokeWeight(this.strokeWeight);
line(start.x, start.y, end.x, end.y);
strokeWeight(3)
Copy link
Copy Markdown
Contributor

@GregStanton GregStanton Jul 21, 2023

Choose a reason for hiding this comment

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

Maybe the midpoint doesn't need to be displayed? Or if it does, would it take long to replace the hard-coded 3 with something else? It might be okay for the sake of the prototype.

Copy link
Copy Markdown
Contributor Author

@tfadali tfadali Jul 21, 2023

Choose a reason for hiding this comment

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

Midpoint display is to help indicate that you can drag it if your mouse is over that point.

point(this.getCenterInCanvas())
pop();
}
}

/**** Arrow ****/
class Arrow {
constructor(w, v, p, hW = 0.5, hL = 0.75) {
Expand Down Expand Up @@ -643,6 +722,16 @@ class Draggable {
}
}

setOffset(offset) {
this.offsetX = offset.x
this.offsetY = offset.y
}

getOffset() {
return createVector(this.offsetX, this.offsetY)

}

//setters
setMouseIsOver(callback) {
this.mouseOverPair[0] = callback;
Expand Down
8 changes: 8 additions & 0 deletions style.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
html, body {
margin: 0;
padding: 0;
}

canvas {
display: block;
}