Skip to content

Week2 - #2

Open
Noratho wants to merge 17 commits into
mainfrom
week2
Open

Week2#2
Noratho wants to merge 17 commits into
mainfrom
week2

Conversation

@Noratho

@Noratho Noratho commented Dec 2, 2020

Copy link
Copy Markdown
Contributor
  • Linked physics and graphics engine
  • a lot of character data structuring
  • created json integration for character data
  • created frame based input detection to detect combination inputs
  • created input parser skeleton
  • created a large chunk of the character json


//Repeat inputs are read every 5-6 frames
switch (event.getCode()) {
case ci::app::KeyEvent::KEY_r:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Instead of pushing back strings, an enumeration would be more readable and faster. Also, don't forget to remove the hardcoded 5.
Also, why did you choose to push the moves back to a list instead of just calling the relevant functions when a key is pressed?

namespace visualizer {


//add color value to customize

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Don't forget that documentation goes in the .h file as a CDoc, not as an inline comment in the .cc file. This also goes for the comment on lines 11-12.
Also, despite your dislike for semantics, yours has actually improved a lot. I'm impressed.


b2Shape::Type shape_type = fixture->GetType();

if (shape_type == b2Shape::e_polygon ) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
if (shape_type == b2Shape::e_polygon ) {
if (shape_type == b2Shape::e_polygon) {


void visualizer::CinderMap::Render() const {

int color_differentiator = 0;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It's acceptable to just use diff in names, for brevity.

Comment thread include/core/move_model.h
@@ -4,24 +4,58 @@

#pragma once

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Notes on commit messages:

  1. Don't repeat them.
  2. Don't write them in first person (don't say "I also...").
  3. If you want to mention two things you did in one commit message (which should ideally only be when the two things were somewhat minor/did not involve a lot of code), you can just separate them with a semi-colon; using "as well as" is fine, but it might cause trouble later when you're trying to keep your message under 50 characters.

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