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

Guiding vectors #140

Merged
merged 9 commits into from Aug 19, 2018
Merged

Guiding vectors #140

merged 9 commits into from Aug 19, 2018

Conversation

davepagurek
Copy link
Member

This adds a cost function which only looks at the structure of shapes.

How it works

  • You specify some bezier curves (visualized in white on top of everything)
  • For any given point in space, the guiding vector for that point is equal to the slope at the closest point on any of the specified bezier curves
  • When a new node is added (we specifically don't look at GeometryNodes), the vector from the node's origin to its parent node's origin is compared to the target vector at that location. The cost is zero if it is exactly aligned.

Good examples

screen shot 2018-08-11 at 12 17 54 pm

screen shot 2018-08-11 at 12 17 51 pm

screen shot 2018-08-11 at 9 13 30 am

Bad examples

This behaviour, while correct, is probably unexpected. The tree grows along the extrapolated vector field lines, but they diverge from the original curve. This can maybe be addressed by adding a falloff function of some kind so that the cost goes up the farther away from a target curve you get, regardless of orientation.

screen shot 2018-08-11 at 12 17 58 pm

Other refactors done in the process

  • Cost functions are classes now. Hopefully that makes them more readable, as they can refactor pieces into private methods. (This also enables them to expose additional methods for visualization purposes.)

Note on Bezier curves

Right now, a bezier curve is defined by four points: The first endpoint, the first control point, the second control point, and the final endpoint.

image

We can maybe add a UI later to adjust these control points. But it's maybe not too bad leaving this as a text input since at least artists are aware of what the control points are from having used Illustrator and conventional 3D modelling programs.

@@ -30,16 +30,19 @@
"lint": "tslint -t stylish --project \"tsconfig.json\"",
"test": "npm run test-only",
"test-only": "jest --coverage",
"watch": "tsc -w -p tsconfig.build.json"
"watch": "tsc -w -p tsconfig.build.json",
"webpack": "webpack"
Copy link
Member Author

Choose a reason for hiding this comment

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

I added this because I had been doing some development in the graphics lab, which has npm but not yarn. Yarn automatically translates rules like this, but npm doesn't.

@davepagurek
Copy link
Member Author

Also: the new cost function classes for FillVolume and ForcePoints are unchanged from before, but now structured as classes with private methods instead of a giant function that initially defines sub-functions. The actual code there hasn't changed.

@armcburney
Copy link
Member

I like the idea about potentially having a UI for the adjusting the control points 👍

@@ -19,3 +19,5 @@ src/**/*.js
src/**/*.js.map
tests/**/*.js
tests/**/*.js.map

*.swp
Copy link
Member

Choose a reason for hiding this comment

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

vim 👎 😂

},
"author": "",
"license": "MIT",
"dependencies": {
"@types/bezier-js": "^0.0.7",
Copy link
Member

Choose a reason for hiding this comment

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

How production ready is bezier-js? Are there any concerns about using this library?

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems pretty legit, the things documented here http://pomax.github.io/bezierjs/ don't seem to have significant active issues (issues are mostly feature requests)

/**
* Creates a cost function based on how much of a target volume a shape fills.
*/
export class FillVolume {
Copy link
Member

Choose a reason for hiding this comment

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

Could we have an abstract base class or interface for Cost, with methods getCost here?

Copy link
Member

Choose a reason for hiding this comment

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

I see that you have the CostFn interface, could you please specify that this class is implementing it?

grid,
(point: string) =>
// If this point was in the target region, reduce the cost
(incrementalCost +=
Copy link
Member

Choose a reason for hiding this comment

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

is this the autoformatter's doing? 🤕 lol

Copy link
Member Author

Choose a reason for hiding this comment

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

yep :')

* @param {number} step The space between field lines
* @returns {Float32Array} A buffer of vertices for the vector field lines.
*/
public generateVectorField(radius: number = 3, step: number = 0.5): Float32Array {
Copy link
Member

Choose a reason for hiding this comment

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

This breaks the Dependency Inversion Principle if this class is supposed to implement the same CostFn interface. Do you think we could refactor this somehow?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure what the best way to do this would be. From SOSMC's point of view, it only uses the getCost method, so it only has access to the CostFn interface. However, there will inevitably be different things that we want to visualize for each type of cost function so I'm not sure how to get them into the same interface for absolutely everything, so exposing extra methods that SOSMC can't see but that we can still pass to the renderer seemed like a decent compromise to me. I'm open to better suggestions though

*
* @returns {Float32Array[]} A vertex buffer for each curve.
*/
public generateGuidingCurve(): Float32Array[] {
Copy link
Member

Choose a reason for hiding this comment

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

Same comment here about the Dependency Inversion Principle

Renderer,
RGBColor,
Shape
} from '../calder';

// tslint:disable-next-line:import-name
import Bezier = require('bezier-js');
Copy link
Member

Choose a reason for hiding this comment

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

Are we going to require the user imports this library in their calder code, or could we abstract this?

Copy link
Member

Choose a reason for hiding this comment

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

I mean, I guess it matters less since we're research focused now, and not user-focused.

Copy link
Member Author

Choose a reason for hiding this comment

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

we could always provide our own wrapper, but yeah for now they do

Copy link
Member

@armcburney armcburney left a comment

Choose a reason for hiding this comment

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

Nice work Dave! This is great! 🎉

@davepagurek
Copy link
Member Author

re: control points UI, if we decide this is the base cost function we want to go with, then we could start out with a UI where you have maybe xyz sliders for each point and you see updates in real time, and then it shows a corresponding BezierJs.Bezier constructor for you to paste in to avoid directly editing the code.

@davepagurek davepagurek merged commit 5188f34 into master Aug 19, 2018
@davepagurek davepagurek deleted the cost-fn-tests branch August 19, 2018 13:12
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.

None yet

2 participants