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

Refactor movement: Further cleanup, added new function #88

Merged
merged 15 commits into from Mar 11, 2019

Conversation

Projects
None yet
3 participants
@gitMarky
Copy link
Contributor

gitMarky commented Mar 6, 2019

This is still not the end, but it is a small step forward :)

gitMarky added some commits Mar 6, 2019

Movement: Start comments with capitals
This was super important!!
Movement: Renamed parameters
Also updated the header file
Movement: DoMovement() simplified digging free the area
There seemed to be no sense in doing the same calculation twice
Movement: Extracted function for digging
Not sure about the function name, but either way this is a
self-contained logical unit and therefore it should be a separate
function.

Tested digging with elevator and the shovel, had no suprises.
Show resolved Hide resolved src/object/C4Movement.cpp Outdated
Show resolved Hide resolved src/object/C4Movement.cpp Outdated
Show resolved Hide resolved src/object/C4Object.h Outdated
@isilkor

This comment has been minimized.

Copy link
Contributor

isilkor commented Mar 7, 2019

In re b69dfec: (Most of) those comments are fine starting with a lower-case letter because they don't start a complete sentence.

@gitMarky

This comment has been minimized.

Copy link
Contributor Author

gitMarky commented Mar 7, 2019

Also, what about moving the MovementDigFreeTargetArea() from C4Object::DoMovement() to C4Object::ExecMovement()?
It feels strange to call this in a function that should do movement, and it does a best guess where the object might end up, based on velocity, and digs free there.

Show resolved Hide resolved src/object/C4Movement.cpp Outdated
Show resolved Hide resolved src/object/C4Movement.cpp Outdated
@isilkor

This comment has been minimized.

Copy link
Contributor

isilkor commented Mar 7, 2019

@gitMarky
Copy link
Contributor Author

gitMarky left a comment

Line 570, I think that the if (has_moved) should not be in the else-block. However, the other update function might also indirectly call UpdatePos currently.

@gitMarky

This comment has been minimized.

Copy link
Contributor Author

gitMarky commented Mar 7, 2019

gitMarky wrote on 07.03.2019 18:41:
Also, what about moving the MovementDigFreeTargetArea() from C4Object::DoMovement() to C4Object::ExecMovement()? It feels strange to call this in a function that should do movement, and it does a best guess where the object might end up, based on velocity, and digs free there.
I think any functional changes should be part of a separate PR.

That is OK, I wanted an opinion before I change and try it out, though.

Movement: Review comment
Renamed contact_count to contact_bits, because it is a bitmask

@gitMarky gitMarky merged commit 3da8956 into openclonk:master Mar 11, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gitMarky gitMarky deleted the gitMarky:refactor branch Mar 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.