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

Create Vector2 Struct #325

Draft
wants to merge 81 commits into
base: master
Choose a base branch
from

Conversation

CloneDeath
Copy link
Contributor

Refactor a lot of xy logic into some basic structures.
This includes a Vector2 and Size struct to hold x,y coordinates, as well as width/height.

Copy link
Collaborator

@matt-gretton-dann matt-gretton-dann left a comment

Choose a reason for hiding this comment

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

Various comments.

I strongly encourage the use of namespaces as appropriate.

I also don't like lots of little header files. A header file should be a logical group of functionality, which is usually larger (in my mind at least) than a single class.

covid-sim.vcxproj Outdated Show resolved Hide resolved
src/Coordinates/Location.hpp Outdated Show resolved Hide resolved
src/Household.hpp Outdated Show resolved Hide resolved
@zebmason
Copy link
Contributor

See #213

@CloneDeath
Copy link
Contributor Author

I strongly encourage the use of namespaces as appropriate.

I placed namespaces around the Geometry and Model classes. I don't think it's great, but it's a start.

I'd like to dive deeper into better namespaces and more organization later, but for now, I think this is a good first step.

@CloneDeath
Copy link
Contributor Author

@matt-gretton-dann

I also don't like lots of little header files.

I think the files in Geometry should be kept apart, as their differences are significant enough, and I wouldn't want people to confuse (for example) a function for Vector2 with Size.

As for the Models, I believe that as we start refactoring more and more functions into these classes, that their disparity will grow. However, as it stands, a few are still very small and similar. So, if you think we should merge some of these classes, let me know, and I'll get on it right away.

@CloneDeath
Copy link
Contributor Author

CloneDeath commented May 26, 2020

@zebmason

See #213

This avoids the issue of bringing in a third party library, but I did like your name Geometry for the namespace.

This PR has gotten very large, but I have skimmed through that PR and saw a few things I'd like to pull over too.

Is there anything specifically you think should go in this PR, and not wait for a followup?

@zebmason
Copy link
Contributor

@CloneDeath I found with #213 that it just started to grow and grow then the merge conflicts turned it to hell. I like the bounding box code that I wrote as it actually flagged up and inconsistency in the code #268

Fixed dist2_cc to use the new Vector math.
Moved distance_to out of Location and into Vector2.
Added explicit cast operator from Size to Vector2.
Moved several variables closer to their usage.
Updated some more vector math.
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.

4 participants