Skip to content

Create Vector2 Struct#325

Draft
CloneDeath wants to merge 81 commits into
mrc-ide:masterfrom
CloneDeath:create-location-struct
Draft

Create Vector2 Struct#325
CloneDeath wants to merge 81 commits into
mrc-ide:masterfrom
CloneDeath:create-location-struct

Conversation

@CloneDeath

Copy link
Copy Markdown
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.

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

Copy link
Copy Markdown
Collaborator

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.

Comment thread covid-sim.vcxproj Outdated
Comment thread src/Coordinates/Location.hpp Outdated
Comment thread src/Household.hpp Outdated
@zebmason

Copy link
Copy Markdown
Contributor

See #213

Comment thread src/Coordinates/Vector2.cpp Outdated
@CloneDeath CloneDeath force-pushed the create-location-struct branch from dbda9b2 to 70edac8 Compare May 26, 2020 22:40
@CloneDeath

Copy link
Copy Markdown
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
Copy Markdown
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

CloneDeath commented May 26, 2020

Copy link
Copy Markdown
Contributor Author

@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
Copy Markdown
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

CloneDeath added 22 commits May 28, 2020 07:01
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.
CloneDeath added 28 commits May 28, 2020 07:01
Moved several variables closer to their usage.
Updated some more vector math.
@CloneDeath CloneDeath force-pushed the create-location-struct branch from fb1b442 to 0767c50 Compare May 28, 2020 12:03
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