Skip to content

Bezier#39

Open
patrickhyw wants to merge 2 commits into
masterfrom
bezier
Open

Bezier#39
patrickhyw wants to merge 2 commits into
masterfrom
bezier

Conversation

@patrickhyw
Copy link
Copy Markdown
Collaborator

No description provided.

elif canonical_degree >= 180:
canonical_degree -= 180
else:
raise FloatingPointError(f'Scene.gps: canonical_degree {canonical_degree} is just wrong')
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

isn't this not possible unless you had a complex number ig

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

it's not possible at all theoretically but I put this here just in case

raise FloatingPointError(f'Scene.gps: canonical_degree {canonical_degree} is just wrong')

canonical_degree = math.floor(canonical_degree)
assert type(canonical_degree) is int
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

does math.floor() ever not return int?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

in Java it doesn't so this is just here in case Python changes math.floor()

# get canonical degree, which is floored and [0, 180)
canonical_degree = math.degrees(ref.get_head_angle())

while not (0 <= canonical_degree < 180):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

isn't this basically turning the head angle into the tail angle? right cause it's basically just shifting the "origin" of the angle measurement from head to tail and geometrically that's basically just transforming head angle into tail angle

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

also I think it'd be better design to make this a part of the Arrow class so that you can reuse it outside of gps if needed (and so that gps doesn't calculate a property of the arrow)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I feel like this is a gps specific thing though not an Arrow thing because only this specific part of GPS needs "either the head or tail, whichever one's between 0 and 180"

# loop through all angle lists
for same_angle_list in ref_by_angle.values():
# for each angle list, loop through all pairs of refs
for i in range(len(same_angle_list)):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this is fine, I just thought I'd drop an alternate way to do it in case you decide you prefer that:

for i, ref1 in enumerate(same_angle_list):
for j, ref2 in enumerate(same_angle_list[i+1:]):
...

although I think slices might be copies so your way avoids some overhead if that's the case but just a thought

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

hmm yeah that's better design according to the problem I graded for ics33

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

slicing doesn't copy actually so I'll change it to that

Comment thread diagrammer/scene/basic.py
TAIL = 'tail'
BEZIER_RADIANS = math.radians(30)

STRAIGHT = 'straight'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

you could add the path settings to ArrowSettings instead maybe

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

that's what I did at first lol but there was some problem with it

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

oh yeah the problem is I have a class variable called smth like PY_REFERENCE_ARROW_SETTINGS that all PyReferences are initialized with, so if I modify it it would modify the settings of all PyReferences.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

and it wasn't good design to allow the user to modify the settings. you could have "get_settings()" function in Arrow and have the user modify that directly, but the problem is when you modify certain settings you need to reset the cache. Thus, you need a "set_settings()" function, but users (as in us) might forget to call set_settings() and just do get_settings() directly. Another solution is to have a set_path() function and no get_settings() function, but that means Arrow has to know about the details of ArrowSettings, which didn't seem right. Overall, it seems like the way we designed settings isn't meant for them to be modified after creation

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.

2 participants