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

Big'N'Veiny pickups #493

Merged
merged 2 commits into from Jun 29, 2018

Conversation

Projects
None yet
4 participants
@husho
Copy link
Contributor

husho commented Jun 1, 2018

@husho husho changed the title Big'N'Veiny pickups Big'n'Veiny pickups Jun 1, 2018

@husho husho referenced this pull request Jun 5, 2018

Closed

[WIP] Rampages #504

1 of 7 tasks complete
@JayFoxRox

This comment has been minimized.

Copy link
Collaborator

JayFoxRox commented Jun 19, 2018

This has not gotten any review the past 2+ weeks. Can you provide more information or even a screenshot to attract reviewers?

I assume this can be tested by starting the http://gta.wikia.com/wiki/Big%27n%27Veiny mission.

Some possible things you could have included in the first post:

  • What's still missing to play the entire Big'n'Veiny mission?
  • How was this implemented? I assume the addresses locations are dumped from gta3.exe?
  • Is there anything missing from this PR for the pickups?
  • Is there any specific behaviour which should be tested?
@@ -81,6 +85,34 @@ class PickupObject : public GameObject {
return false;
}

virtual bool isBigNVeinyPickup() const {
return false;
}

This comment has been minimized.

@JayFoxRox

JayFoxRox Jun 19, 2018

Collaborator

nit: This does look slightly out of place.
Does GTA3 really do it this way? I'd have expected some kind of enum to identify the pickups by type, instead of such a very specific getter which is only useful for one single item type, used in a single mission of the game.

This comment has been minimized.

@husho

husho Jun 19, 2018

Contributor

GTAIII does this very poorly, there is separate global pool for these pickups, and separate update, render functions, and special creation and removal functions for opcodes.

This comment has been minimized.

@husho

husho Jun 19, 2018

Contributor

There is no, like, actual pickup type, which describes what this pickup is, in original game. We already have behavior specific type variable [same as in original game]. GTAIII just uses model index for that. For example if behavior type is PICKUP_ONCE or PICKUP_ONCE_TIMEOUT, etc it checks for model if it's health pickup model then give health. I guess we can add type enum, and ditch all these special classes maybe?

This comment has been minimized.

@JayFoxRox

JayFoxRox Jun 19, 2018

Collaborator

This is fine with me then (for now).

I guess we could have something like a PickupPool struct in the future which could be more similar to what GTA3 does.

But I feel this is such a small portion of the project that it's not worth time to redesign anything at this point. The solution is ugly, but it dos work.

@JayFoxRox

This comment has been minimized.

Copy link
Collaborator

JayFoxRox commented Jun 19, 2018

I guess you can squash the "Minor fixes" commit into the first commit, or give the commit a better title which is more descriptive. Theoretically most commits could be named "Minor fixes" or "Major fixes" - so avoid such generic names.

I've skimmed over the code and it looks ok. I did not test it yet and I'm waiting for more information until I feel confident to give my final approval.

@husho

This comment has been minimized.

Copy link
Contributor

husho commented Jun 19, 2018

What's still missing to play the entire Big'n'Veiny mission?

It's almost playable, mission softlocks in the end, due to missing AI.

How was this implemented? I assume the addresses are dumped from gta3.exe?

Yes

Is there anything missing from this PR for the pickups?

If you are talking about only Big'N'Veiny pickups then I don't think so.

Is there any specific behaviour which should be tested?

There is no special behaviour, except they only can be picked up in vehicle.

I guess you can squash the "Minor fixes" commit into the first commit

Will do

@husho husho changed the title Big'n'Veiny pickups Big'N'Veiny pickups Jun 19, 2018

@@ -507,10 +507,13 @@ void RWGame::tick(float dt) {
clockAccumulator -= 1.f;
}

constexpr float timerClockRate = 1.f / 25.f;

This comment has been minimized.

@JayFoxRox

JayFoxRox Jun 19, 2018

Collaborator

I don't fully understand this change yet (I'm also not aware of the surrounding code).

Where does this / 25.f come from - gta3.exe?
Does it have to do with FPS or what's the story behind it?

also nit for this commit: commit title is too long; should be reworded to not break into commit description

This comment has been minimized.

@husho

husho Jun 19, 2018

Contributor

Does it have to do with FPS or what's the story behind it?

Basically, yes, it's tied to FPS.

In GTAIII timers are updated every frame and framerate is locked to 30fps [if you have frame limiter on], there is no accumulators or anything. They may go out of sync depending on game's uptime, due to double precision.

bool onPlayerTouch() override;
};

const static std::array<glm::vec3, 106> bigNVeinyPickupsLocations = {

This comment has been minimized.

@ShFil119

ShFil119 Jun 24, 2018

Member

nit: Can it be constexpr? It should be doable on almost every distro. (GLM 0.9.8.0 - 2016-09-11)

@danhedron

This comment has been minimized.

Copy link
Member

danhedron commented Jun 25, 2018

LGTM

husho added some commits Jun 1, 2018

Fixed: tests weren't working
Fixed: mission timer
Timer was going crazy due to missing 02d9 opcode, update timer 25 times per second

Fixed: mission timer
Don't beep on every timer update

@danhedron danhedron merged commit 4c357ad into rwengine:master Jun 29, 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:pacman branch Jun 29, 2018

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