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

Added Architecture and Design Doc #1012

Merged

Conversation

MathewMacDougall
Copy link
Contributor

@MathewMacDougall MathewMacDougall commented Nov 4, 2019

Please fill out the following before requesting review on this PR

Description

Added a big document that explains our architecture, system design, important components, and design patterns.

I also tweaked all the other docs to either fix links, or make minor updates.

I'm tagging more people than usual for review, since this will really benefit from more eyes and opinions. Please comment if anything isn't clear, if I missed anything, etc. We really want this documentation to be solid so it's useful to everyone.

The documentation focuses more on higher-level concepts and ideas, rather than any specific implementations since those can be read in the code, and we don't want to have to update the documentation often. That being said please let me know if you think sections need more detail.

HOW TO REVIEW

Reading raw markdown isn't easy. I'd recommend clicking "View file" for each changed file and just reading the nicely formatted markdown. If you have something to comment on, then go back and find the closest change and leave a comment there.

Testing Done

N/A. Manually made sure as many links work as possible.

Resolved Issues

resolves #878
resolves #879
resolves #880

Length Justification

The architecture doc is pretty big, and I wanted to do everything all at once to make linking easier.

Review Checklist

It is the reviewers responsibility to also make sure every item here has been covered

  • Function & Class comments: All function definitions (usually in the .h file) should have a javadoc style comment at the start of them. For examples, see the functions defined in thunderbots/software/geom. Similarly, all classes should have an associated Javadoc comment explaining the purpose of the class.
  • Remove all commented out code
  • Remove extra print statements: for example, those just used for testing
  • Resolve all TODO's: All TODO (or similar) statements should either be completed or associated with a github issue
  • Justify drops in code coverage: If this PR results in a non-trivial drop in code coverage (a bot should post a coverage diagram as a comment), please justify why we can't test the code that's not covered.

Combined the convention diagrams
@MathewMacDougall
Copy link
Contributor Author

can we consolidate the angle and coordinate convention diagrams? Makes it easier to maintain, and I think there's plenty of space for both

done

@MathewMacDougall
Copy link
Contributor Author

Made a bunch of changes, ready for re-review

Copy link
Contributor

@jonathanlew jonathanlew left a comment

Choose a reason for hiding this comment

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

👍

jonathanlew
jonathanlew previously approved these changes Nov 11, 2019
EvanMorcom
EvanMorcom previously approved these changes Nov 11, 2019
Copy link
Contributor

@EvanMorcom EvanMorcom left a comment

Choose a reason for hiding this comment

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

LGTM 💯

Copy link
Contributor

@garethellis0 garethellis0 left a comment

Choose a reason for hiding this comment

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

Could we indicate on the coordinate_and_angle_convention_diagram where (0,0) is? Would also be helpful (but not necessary) to indicate which of {x,y} are positive/negative in each quadrant.

@MathewMacDougall
Copy link
Contributor Author

Updated the coordinate_and_angle_convention_diagram

Copy link
Contributor

@garethellis0 garethellis0 left a comment

Choose a reason for hiding this comment

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

LGTM 👍 🎉

@MathewMacDougall
Copy link
Contributor Author

@scveloso @matthewberends @jared-paul need re-review (and approval)

Copy link
Contributor

@matthewberends matthewberends left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@LiCody LiCody left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@hardik-ddod hardik-ddod left a comment

Choose a reason for hiding this comment

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

Looks good!

@MathewMacDougall
Copy link
Contributor Author

@scveloso @jared-paul can you please take another look so we can wrap this up?

Copy link
Contributor

@amrm3lm amrm3lm left a comment

Choose a reason for hiding this comment

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

informative, LGTM

Copy link
Contributor

@HaomiaoZ HaomiaoZ left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Contributor

@jared-paul jared-paul left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@MathewMacDougall MathewMacDougall merged commit 61f1f73 into UBC-Thunderbots:master Nov 15, 2019
@MathewMacDougall MathewMacDougall deleted the mathew/architecture_doc branch November 15, 2019 05:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet