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

Preliminary implementation of Measure #159

Merged
merged 64 commits into from
Apr 7, 2014
Merged

Preliminary implementation of Measure #159

merged 64 commits into from
Apr 7, 2014

Conversation

jeffreyrosenbluth
Copy link
Member

NOT ready to merge
In particular arrows only use output units.
Companion Branches: core - units, svg - units

@jeffreyrosenbluth
Copy link
Member Author

The basic idea seems to be working. I would like others to have a look before we try to attack delayed leaves. In particular, I made some executive decisions about where things should go and how general/specific we should be. Of course all open to discussion.

Note all arrow related import are commented out - so Arrow.hs and ArrowHead.hs are not compiled.

@bergey
Copy link
Member

bergey commented Feb 13, 2014

Looks good, other than my questions above. Thanks!

@jeffreyrosenbluth
Copy link
Member Author

All of the Measure types work as expected.
The backends must now implement renderRTree.
I'm sure there can still be improvements and simplifications but I think we are ready to work on arrows and delayed leaves.

@jeffreyrosenbluth
Copy link
Member Author

Arrows now working. Getting arrows to work was laughably easy. The combination of removing freeze and the power of delayed leaves means there is no longer any need for scaleInv. Hence it has been eliminated from arrows. There are some other things we may want to add, in particular arrow heads and tails sizes are now in Output units, we should consider adding other units. Also arrows now have an empty envelope and trail, this may or may not be the semantics we want. Of course more testing is necessary. The test program ArrowTest.hs in -lib/test has been modified use in the branch.

@bergey
Copy link
Member

bergey commented Mar 14, 2014

I finally had some time to understand the arrow code and think about adding Measure.

My first thought was to pass DelayedLeaf information about the global-to-normalized and normalized-to-output transforms. I don't really like this. It changes the semantics in ways I haven't thought through, and I'm not sure what these extra Transformations mean in dimensions above 2.

Maybe it's simpler to make arrow tail size and head size Attributes? DelayedLeafs already receive the Style, and we can use the same machinery for converting Measure to Output in LineWidth & FontSize. @jeffreyrosenbluth, do you think this would work, or am I missing something? The current code evaluates the DelayedLeaf as part of toDTree, but converts the Measure by walking the RTree, so we'll need to change that around.

It's a bit awkward if you're drawing a bunch of arrows - you can reuse the ArrowOpts, but still need to set the Style either for each arrow or above them all in the tree.

FontSize is already an Attribute, so upgrading it to Measure should be easy.

@jeffreyrosenbluth
Copy link
Member Author

I really believe that we have to think carefully about the tradeoffs between having the full set of Measure options for arrows and the awkwardness you mention. It just may not be worth having full generality if the arrow api is clumsy. Especially if it is to introduce Measures that are not used.

That being said, and I have to think this through more carefully, my intuition is that this would work. The reason scaleInv was introduced was to accommodate non-uniform scalings, but conversion between different measures are always uniform so I don't think we will have arrowheads at the wrong angle, like we would have previously with non-uniform scalings.

I do like the idea of making tail size and head size attributes, if possible. But we need to think carefully about the joints, as they depend on both the head size and shaft width. Honestly, some of these things we wont know until we try to code it.

I'm not sure what you have in mind regarding where DelayedLeaf and Measure are handled. Since backends don't get to see the DTree, the only place that Global measure can be handled is at the RTree level.

@bergey
Copy link
Member

bergey commented Mar 14, 2014

In principle we can walk the QDiagram and modify the styles, but writing that map / traversal in a general way that respects the invariants or has clear usage constraints looks hard. @haasn seems willing to tackle it, eventually. For this branch, I think we can get away with adding a Style v -> Style v parameter to toRTree and toDTree and threading information about measured Attributes that way.

@jeffreyrosenbluth
Copy link
Member Author

Just to make sure i understand. headSize and tailSize would no longer be fields of the ArrowOpts but would be set like normal attributes?

@bergey
Copy link
Member

bergey commented Mar 14, 2014

Yes that's my suggestion. I think it works for the DelayedLeaf, but it may cause problems for your new Traversals, since they don't have access to the Style. I'd like to write some code and see what breaks, but I probably won't have time to try it out until Tuesday at the earliest. If you have time before that, that would be great.

@jeffreyrosenbluth
Copy link
Member Author

I think its worth a shot, I probably wont have much time until the middle of next week as well. There is no rush so why don't you give it a go.

@jeffreyrosenbluth
Copy link
Member Author

I do think the semantics we achieve with the new Traversals is important, but there are probably other ways we can achieve the same results.

bergey added 3 commits March 16, 2014 01:04
as Attributes, they can be defined in terms of Measure, and easily
converted to Output during rendering
add new module Diagrams.TwoD.Compile for rewriting RTree, to avoid
import loop
@bergey
Copy link
Member

bergey commented Mar 16, 2014

That's my draft. @jeffreyrosenbluth, take a look when you have some time, poke holes in it.

@jeffreyrosenbluth
Copy link
Member Author

Nice, I should have time to look at it today. Does it compile? Have you run it on any examples?

@bergey
Copy link
Member

bergey commented Mar 16, 2014

It builds with -Wall -Werror, but I haven't run it on examples. And I haven't done anything in the Backends.

jeffreyrosenbluth and others added 27 commits March 25, 2014 16:26
was going to be too much work to fix it up for changes to Backend class,
and it only ever half-worked anyway.  If we ever want to revive it, it's
there in the git history.
implement strut and phantom the right way: using leafU
Updates to work with `Backend` redesign
byorgey added a commit that referenced this pull request Apr 7, 2014
* Updates to use `Measure`
* Get rid of bitrotted `Show` backend
@byorgey byorgey merged commit 6dbd6cf into master Apr 7, 2014
@byorgey byorgey deleted the units branch April 7, 2014 14:06
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