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

Coordinate transforms are a hassle #134

Open
Ichoran opened this issue Feb 27, 2017 · 6 comments
Open

Coordinate transforms are a hassle #134

Ichoran opened this issue Feb 27, 2017 · 6 comments
Milestone

Comments

@Ichoran
Copy link
Contributor

Ichoran commented Feb 27, 2017

The "centroid can act as the origin" scheme I proposed turns out to considerably complicate the implementations.

I propose that we do away with the dual function of centroid:

  • The origin is the only origin. All coordinates are relative to it, if it is present. It is (0, 0) if not present.
  • The centroid is kept track of if present, but it doesn't do anything.
@MichaelCurrie
Copy link

Given that we've already implemented this feature, are we sure it's a good idea to go back and change it now? Is it causing active problems for you?

Thanks

@Ichoran
Copy link
Contributor Author

Ichoran commented Feb 28, 2017

I was rewriting for perimeters and noticing that this feature made it a considerably trickier rewrite than if it wasn't there.

It's not a fatal flaw. It's just an extra burden on other people who implement this which is maybe not called for.

But the "leave it alone already" sentiment is compelling too, which is why I wanted to raise the issue.

@Ichoran
Copy link
Contributor Author

Ichoran commented Mar 2, 2017

@MichaelCurrie - The problem with origins as they stand now is that there is no way to avoid the feature without getting gobbledygook. You absolutely must parse the correct origin in order for the spine positions to make any sense. So it's absolutely a required feature for a reader (not a writer) to properly interpret the numbers.

That's why I think it's worth the effort to make the change.

If there is an origin "ox", "oy", all other values are relative to that. If not, all other values are absolute. This way readers don't even have to care whether cx and cy exist.

Again, I don't like making a change like this so late, but I really think simplicity is to be strived for with the basic feature set. Doing without an origin can dramatically increase file sizes, though, so I don't think we can mandate that everything be global (which would be even simpler).

@Ichoran
Copy link
Contributor Author

Ichoran commented Mar 16, 2017

The simplest version (ox/oy are either the same dimension as t, or are absent) is now documented in the readme; see #146

@MichaelCurrie MichaelCurrie added this to the python_1.2.0 milestone Jun 18, 2017
@Ichoran
Copy link
Contributor Author

Ichoran commented Jun 26, 2017

@MichaelCurrie - I'm not familiar with any transformations you might do in the Python code. This PR said that cx, cy should no longer be used as an offset for x, y values (i.e. only ox, oy would do that). Does this impact the code, or were these values always just passed on to the user? Either way, does the latest Python PR solve this issue?

@MichaelCurrie
Copy link

MichaelCurrie commented Jun 28, 2017

I believe it does impact the code: I believe the code will offset x,y by cx,cy if ox,oy are not present, but I will need to check this in more detail.

The latest Python PR did not solve this issue, if it is indeed present; this and contours are what's remaining for me to finish the Python 1.2 release. I'll be able to get to this next week.

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

No branches or pull requests

2 participants