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

[Ready] Garages continuation #490

Merged
merged 1 commit into from Jun 25, 2018

Conversation

Projects
None yet
5 participants
@husho
Copy link
Contributor

husho commented May 31, 2018

#107

  • Get rid of GarageInfo and GarageController and replace them with one class
  • Opcodes (most of them)
  • Fix swing garages
  • Door height based on model size
  • Fix isObjectInsideGarage to check spheres
  • Second door support
  • Fix segfault
  • Fix double doors
  • Mission garages (almost full functionality)
  • Improved respray garages

@husho husho changed the title [WIP] Garages continuation [Ready] Garages continuation Jun 1, 2018


void Garage::updateDoor() {
if (swingType) {
doorObject->setRotation(glm::angleAxis(fraction * 1.57079632679f,

This comment has been minimized.

@ShFil119

ShFil119 Jun 1, 2018

Member

Originally hardcoded value?

This comment has been minimized.

@husho

husho Jun 1, 2018

Contributor

yes, 90 degrees

This comment has been minimized.

@paulkocialkowski

paulkocialkowski Jun 1, 2018

Contributor

Maybe it would be clearer to put it as either "pi / 2", "2 * pi * 90 / 360" or "pi * 90 / 180"?

This comment has been minimized.

@ShFil119

ShFil119 Jun 1, 2018

Member

You can use glm::half_pi. ;)

@ShFil119

This comment has been minimized.

Copy link
Member

ShFil119 commented Jun 7, 2018

Code looks good, but in my opinion you should add some (dump) unit tests. Without them it's hard to check if code is working or not.

@husho

This comment has been minimized.

Copy link
Contributor

husho commented Jun 8, 2018

I'll do tests in separate PR as well as extend functionality that can be actually tested.

case Type::Respray: {
// Find out real value
garageTimer = engine->getGameTime() + 2.f;
playerVehicle->setHealth(1000.f);

This comment has been minimized.

@ShFil119

ShFil119 Jun 17, 2018

Member

Shouldn't we somehow assert/check there's playerVehicle?

This comment has been minimized.

@husho

husho Jun 17, 2018

Contributor

debug assert will do

This comment has been minimized.

@JayFoxRox

JayFoxRox Jun 19, 2018

Collaborator

There doesn't seem to be one? Only RW_UNUSED. I think we should get rid of playerIsInVehicle in this function, and explicitly check if playerVehicle is valid here.

This comment has been minimized.

@husho

husho Jun 19, 2018

Contributor

Probably it would be used in the future for other garage types.


void Garage::updateDoor() {
if (swingType) {
doorObject->setRotation(glm::angleAxis(fraction * glm::radians(90.f),

This comment has been minimized.

@ShFil119

ShFil119 Jun 17, 2018

Member

Maybe here also?

This comment has been minimized.

@husho

husho Jun 17, 2018

Contributor

Already checked at runtime

This comment has been minimized.

@husho

husho Jun 17, 2018

Contributor

It would be better to replace runtime check with debug assert in constructor when it tries to find door object

This comment has been minimized.

@husho

husho Jun 17, 2018

Contributor

Don't know what error handling pattern we should use, maybe it even better to use std::optional

@@ -5,3 +5,4 @@ documentation/
*swp
*.user*
.DS_Store
.vscode/

This comment has been minimized.

@JayFoxRox

JayFoxRox Jun 19, 2018

Collaborator

Such things don't belong in a project gitignore, but your private git configuration.
As vscode is a popular editor, I'd be willing to allow this here. But ideally it wasn't here.

void updateDoor();

public:
enum class Type {

This comment has been minimized.

@JayFoxRox

JayFoxRox Jun 19, 2018

Collaborator

nit: if these are constants the real game uses there could be a comment about this here.

if (p.z < min.z) return false;
if (p.x > max.x) return false;
if (p.y > max.y) return false;
if (p.z > max.z) return false;

This comment has been minimized.

@JayFoxRox

JayFoxRox Jun 19, 2018

Collaborator

nit: this sort of check is a recurring pattern and I wonder if we should have a function for it (if there isn't one already somewhere)

auto playerVehicle = plyChar->getCurrentVehicle();
bool playerIsInVehicle = playerVehicle != nullptr;
RW_UNUSED(playerPosition);
RW_UNUSED(playerIsInVehicle);

This comment has been minimized.

@JayFoxRox

JayFoxRox Jun 19, 2018

Collaborator

See other comment about playerIsInVehicle. Not sure how useful creating all those variables is when there is no user (and it's unlikely there'll be one).
It's probably more important to make it obvious that we check for the valitity of playerVehicle when it's being used.

(Other instances too, whenever playerIsInVehicle is not actually used - but even if it's used, I'd consider moving it closer to where it's used: Keep playerVehicle as variable intended for use and playerIsInVehicle as a helper)

This comment has been minimized.

@husho

husho Jun 19, 2018

Contributor

Probably it would be used in the future for other garage types.

@@ -1,702 +0,0 @@
#include "GarageController.hpp"

This comment has been minimized.

@JayFoxRox

JayFoxRox Jun 19, 2018

Collaborator

For future changes: When renaming files manually git does not automatically keep track of previous history. If you had used git mv GarageController.cpp Garage.cpp, then git should have created a much much nicer history / diff.
That would also potentially have helped in review.

Fixing this retroactively is a bit harder, but theoretically possible (essentially manually changing it back, and then using git mv, then force push - do at your own risk)

This comment has been minimized.

@JayFoxRox

JayFoxRox Jun 19, 2018

Collaborator

Nevermind, GitHub might just handle this poorly - in the "Big Refactor" commit this is displayed properly.

@JayFoxRox

This comment has been minimized.

Copy link
Collaborator

JayFoxRox commented Jun 19, 2018

I'll do tests in separate PR as well as extend functionality that can be actually tested.

Ideally, this should have been in the PR description, so everyone knows your intentions and can focus review on what's important.


The git history is not ideal (looking at the commit names alone, it's very hard to tell what's going on).
Unfortunately the move of the source file in particular makes review a lot harder than it should be.

However, skimming over the code it looks mostly fine. I have not tried to run this yet.
If it doesn't break the game, I'd like to merge this soon, as it's better-than-master, and @husho promised unit tests, so we don't want to delay those.

So if someone could test this PR, that would be nice. In order to help with testing, it would be nice if @husho could add more information how to test this to the first post / PR description too (and I'd expect that for future PRs to speed up review): which missions, which garages, ...


I suggest creating an issue about missing garage features or unit tests after merge, so that this is not forgotten in case @husho disappears for whatever reasons.

I'd also appreciate if someone could create replacement issues for #107 which reflects the changes in this code. I'd like to close #107 as it's a meta issue which is hard to maintain.

(Ideally these requests for updated issues should also have been mentioned in the first post / PR description, so it's easier for maintainers to keep track of what to do / look for after a merge)

src/engine/Payphone.cpp
src/engine/Payphone.hpp
src/engine/Garage.cpp

This comment has been minimized.

@ShFil119

ShFil119 Jun 21, 2018

Member

Looks like it breaks alphabetical order.

RW_UNUSED(args);
garage->makeDoorSwing();

This comment has been minimized.

@ShFil119

ShFil119 Jun 21, 2018

Member

Unneeded tab?

}
}

float Garage::getDistanceToGarage(glm::vec3 point) {

This comment has been minimized.

@ShFil119

ShFil119 Jun 21, 2018

Member

Arg can be const.

return info;
Garage* GameWorld::createGarage(const glm::vec3 coord0, const glm::vec3 coord1,
Garage::Type type) {
int id = garages.size();

This comment has been minimized.

@ShFil119
#include <engine/Payphone.hpp>
#include <engine/Garage.hpp>

This comment has been minimized.

@ShFil119
class Payphone;
class Garage;

This comment has been minimized.

@ShFil119

ShFil119 Jun 22, 2018

Member

Also order.

max.y = std::max(coord0.y, coord1.y);
max.z = std::max(coord0.z, coord1.z);

glm::vec2 midpoint;

This comment has been minimized.

@ShFil119

ShFil119 Jun 22, 2018

Member

It can also be const glm::vec2 midpoint{(min.x + max.x) / 2, (min.y + max.y) / 2};

midpoint.y = (min.y + max.y) / 2;

// Find door objects for this garage
for (auto p : engine->instancePool.objects) {

This comment has been minimized.

@ShFil119
continue;
}

auto instPos = inst->getPosition();

This comment has been minimized.

@ShFil119

ShFil119 Jun 22, 2018

Member

And here and below?


Garage(GameWorld* engine_, const int id_, const glm::vec3 coord0,
const glm::vec3 coord1, const Type type_);
~Garage();

This comment has been minimized.

@ShFil119

ShFil119 Jun 23, 2018

Member

= default;?


void makeDoorSwing();

bool isObjectInsideGarage(GameObject* object);

This comment has been minimized.

@ShFil119

ShFil119 Jun 23, 2018

Member

Can this and below be const?

husho
@ShFil119
Copy link
Member

ShFil119 left a comment

LGTM, been testing mission Don't Spank My Bitch Up. Now garage (place where we leave Stallion) works as should.
(Door for respraying room doesn't work yet, I assume it'll be done in future pull?)

@danhedron

This comment has been minimized.

Copy link
Member

danhedron commented Jun 25, 2018

LGTM;

I ran into a few issues with the behaviour so I'll log those. They can be fixed in (smaller) followups.

@danhedron danhedron merged commit 7d74003 into rwengine:master Jun 25, 2018

2 checks passed

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

@husho husho deleted the husho:garages_continuation branch Jun 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment