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] Payphones #498

Merged
merged 9 commits into from Jun 18, 2018

Conversation

Projects
None yet
3 participants
@husho
Copy link
Contributor

husho commented Jun 2, 2018

Payday For Ray is now playable

To do:

  • Payphone class
  • Get world object
  • Figure out how games finds z coord
  • Figure out states
  • Opcodes (only that used in main.scm)
  • Basic functionality
  • ~~~Payphone wiggle animation~~~
  • Widescreen
  • Use #476
  • Player animations (talking animation seemed to be bugged)
  • Restore from saves (very basic, just so the game won't crash)
  • Fix talking anim
  • Face player towards a payphone

requires #476

@@ -1257,6 +1257,12 @@ bool SaveGame::loadGame(GameState& state, const std::string& file) {
}
}

// @todo restore properly
for (size_t p = 0; p < payphoneData.numPayphones; ++p) {

This comment has been minimized.

@ShFil119

ShFil119 Jun 2, 2018

Member

It can be done with range loop.

This comment has been minimized.

@husho

husho Jun 2, 2018

Contributor

I don't really wanna touch SaveGame.cpp, as it was being refactored by @danhedron back in February. But there is no progress since :(

Block8Data payphoneData;
READ_VALUE(payphoneData);
std::vector<Block8Payphone> payphones(payphoneData.numPayphones);
for (size_t p = 0; p < payphoneData.numPayphones; ++p) {

This comment has been minimized.

@ShFil119

ShFil119 Jun 2, 2018

Member

Also it can be done with range loop.

This comment has been minimized.

@darkf

darkf Jun 17, 2018

Collaborator

I second this.


void enable();
void disable();
bool isTalking();

This comment has been minimized.

@ShFil119

ShFil119 Jun 2, 2018

Member

const

void enable();
void disable();
bool isTalking();
void setMessageAndStartRinging(std::string m);

This comment has been minimized.

@ShFil119

ShFil119 Jun 2, 2018

Member

Maybe passing by const ref or perfect forwarding will be better?

std::cout << " " << uint16_t(phone.state) << " " << phone.position.x
<< " " << phone.position.y << " " << phone.position.z
std::cout << "Payphones: " << payphoneData.numPayphones << std::endl;
for (size_t p = 0; p < payphoneData.numPayphones; ++p) {

This comment has been minimized.

@ShFil119

ShFil119 Jun 2, 2018

Member

also?

@@ -459,6 +459,15 @@ GarageInfo* GameWorld::createGarage(const glm::vec3 coord0,
return info;
}

Payphone* GameWorld::createPayphone(const glm::vec2 coord) {
int id = payphones.size();
Payphone* payphone = new Payphone(this, id, coord);

This comment has been minimized.

@ShFil119

ShFil119 Jun 2, 2018

Member

Memory leak?

This comment has been minimized.

@ShFil119

ShFil119 Jun 2, 2018

Member

Maybe it's better to keep in vector unique_ptrs?

This comment has been minimized.

@husho

husho Jun 2, 2018

Contributor

But it's not uniquely-owned

This comment has been minimized.

@ShFil119

ShFil119 Jun 2, 2018

Member

You can create unique_ptr with this memory when inserting into vector. Other way is usage of std::move.

This comment has been minimized.

@husho

husho Jun 2, 2018

Contributor

But we still need to return back valid object after inserting into vector, and it's no longer valid after std::move Or I guess you can just do back on vector to return newly created object.

This comment has been minimized.

@ShFil119

ShFil119 Jun 2, 2018

Member

Easiest way payphones.push_back(std::unique_ptr(payphone));
But using back should be more clear what's going on. And we can use native std::make_unique.

This comment has been minimized.

@husho

husho Jun 2, 2018

Contributor

That's all nice, but it doesn't really go well with all this ScriptObjectType stuff, as ScriptObjectType uses raw pointers.

This comment has been minimized.

@ShFil119

ShFil119 Jun 2, 2018

Member

Raw ptr to memory allocated by unique_ptr you can get using get.

This comment has been minimized.

@husho

husho Jun 2, 2018

Contributor

Doesn't that defeat the whole purpose of having a unique_ptr then?

This comment has been minimized.

@ShFil119

ShFil119 Jun 2, 2018

Member

Nope, there's only one owner of memory(so overhead as raw ptr) and you have quarantine that memory is freed at the end.

Good habit is to use ref instead of raw ptr when borrowing memory. As I suggested on IRC lately.

@@ -58,6 +66,23 @@ class PlayerController : public CharacterController {
// @todo not implemented yet
bool isBusted() const;

void pickUpPayphone();

This comment has been minimized.

@ShFil119

ShFil119 Jun 2, 2018

Member

Here and below some kind of comments?

Payphone(GameWorld* engine_, const int id_, const glm::vec2 coord);
~Payphone();

void enable();

This comment has been minimized.

@ShFil119

ShFil119 Jun 2, 2018

Member

Here and below some kind of comments?

@husho husho changed the title [WIP] Payphones [Ready] Payphones Jun 3, 2018

@ShFil119

This comment has been minimized.

Copy link
Member

ShFil119 commented Jun 7, 2018

Also here unit tests would be useful.

husho added some commits Jun 1, 2018

}

Payphone(GameWorld* engine_, const int id_, const glm::vec2 coord);
~Payphone();

This comment has been minimized.

@ShFil119

ShFil119 Jun 8, 2018

Member

maybe better ~Payphone() = default;?

message.clear();
}

Payphone::~Payphone() {

This comment has been minimized.

@ShFil119

ShFil119 Jun 8, 2018

Member

And this remove.

husho
@darkf

This comment has been minimized.

Copy link
Collaborator

darkf commented Jun 12, 2018

Pinging to remind you that the build fails and there are unaddressed comments

@husho

This comment has been minimized.

Copy link
Contributor

husho commented Jun 12, 2018

@darkf review #476 then. And again if SaveGame is going to be refactored to use streams why bother with it?

@darkf

This comment has been minimized.

Copy link
Collaborator

darkf commented Jun 13, 2018

@husho What's the relation between this and #476? ... And you can always change the save stuff later.

@husho

This comment has been minimized.

Copy link
Contributor

husho commented Jun 13, 2018

I used code from #476, it needs to get merged first, so this can be rebased.

@husho

This comment has been minimized.

Copy link
Contributor

husho commented Jun 13, 2018

Also it's not me who worked on save stuff. Check danhedron save-streams branch

RW_UNUSED(args);
payphone->setMessageAndStartRinging(text);

This comment has been minimized.

@ShFil119

ShFil119 Jun 13, 2018

Member

Does it work (I mean payphone is const)?

This comment has been minimized.

@husho

husho Jun 13, 2018

Contributor

Pointer to the object is unchangeable, not the object itself, object method declaration implies that object can be mutated (no const).

RW_UNUSED(args);
payphone->disable();

This comment has been minimized.

@ShFil119

ShFil119 Jun 13, 2018

Member

Does it work (I mean payphone is const)?

This comment has been minimized.

@husho

husho Jun 13, 2018

Contributor

Pointer to the object is unchangeable, not the object itself, object method declaration implies that object can be mutated (no const).

RW_UNUSED(args);
payphone->enable();

This comment has been minimized.

@ShFil119

This comment has been minimized.

@husho

husho Jun 13, 2018

Contributor

Pointer to the object is unchangeable, not the object itself, object method declaration implies that object can be mutated (no const).

@@ -459,6 +459,12 @@ GarageInfo* GameWorld::createGarage(const glm::vec3 coord0,
return info;
}

Payphone* GameWorld::createPayphone(const glm::vec2 coord) {
int id = payphones.size();
payphones.push_back(std::make_unique<Payphone>(this, id, coord));

This comment has been minimized.

@darkf

darkf Jun 17, 2018

Collaborator

Perhaps use emplace_back here?

continue;
}
if (glm::distance(coord, glm::vec2(o->getPosition())) < 2.f) {
object = static_cast<InstanceObject*>(o);

This comment has been minimized.

@darkf

darkf Jun 17, 2018

Collaborator

If you're not going to find the closest payphone here then you should break;

}
}

position = object->getPosition();

This comment has been minimized.

@darkf

darkf Jun 17, 2018

Collaborator

Doesn't enure object isn't null

Block8Data payphoneData;
READ_VALUE(payphoneData);
std::vector<Block8Payphone> payphones(payphoneData.numPayphones);
for (size_t p = 0; p < payphoneData.numPayphones; ++p) {

This comment has been minimized.

@darkf

darkf Jun 17, 2018

Collaborator

I second this.

@@ -122,6 +122,32 @@ void CharacterObject::destroyActor() {
glm::vec3 CharacterObject::updateMovementAnimation(float dt) {
glm::vec3 animTranslate{};

if (controller) {
const auto& c = static_cast<PlayerController*>(controller);

This comment has been minimized.

@darkf

darkf Jun 17, 2018

Collaborator

No need to take a reference to a pointer.

RW_CHECK(param.isLvalue(), "Non lvalue passed as object");
auto& payphones = getWorld()->payphones;
Payphone* payphone = nullptr;
if (size_t(*param.handleValue()) < payphones.size()) {

This comment has been minimized.

@darkf

darkf Jun 17, 2018

Collaborator

Should check if the index is >= 0 as well, also please call this index or something.

This comment has been minimized.

@husho

husho Jun 17, 2018

Contributor

size_t is unsigned, also this code was copied from other function, so what? do I change names in all similar functions then?

This comment has been minimized.

@darkf

darkf Jun 17, 2018

Collaborator

At least here because it's used more than once and it makes it clear it's being used as an index.

And what I mean is the index (*param.handleValue()) could be < 0 from the script, before the cast. Should probably be an assertion.

This comment has been minimized.

@husho

husho Jun 18, 2018

Contributor

What about using std::vector::at

husho added some commits Jun 17, 2018

husho
husho
husho
husho
husho
Ugh
husho

@darkf darkf merged commit 6d04746 into rwengine:master Jun 18, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@@ -461,7 +461,7 @@ GarageInfo* GameWorld::createGarage(const glm::vec3 coord0,

Payphone* GameWorld::createPayphone(const glm::vec2 coord) {
int id = payphones.size();
payphones.emplace_back(this, id, coord);
payphones.emplace_back(make_unique<Payphone>(this, id, coord));

This comment has been minimized.

@ShFil119

ShFil119 Jun 18, 2018

Member

Didn't you forget include memory?

This comment has been minimized.

@husho

husho Jun 18, 2018

Contributor

It's in the header file

This comment has been minimized.

@ShFil119

ShFil119 Jun 18, 2018

Member

Yeah, but usage lacks std::.

This comment has been minimized.

This comment has been minimized.

@husho

husho Jun 18, 2018

Contributor

I dont understand why it was merged before #476 even

@husho husho deleted the husho:phones branch Jun 18, 2018

NFSMONSTR added a commit to NFSMONSTR/openrw that referenced this pull request Jun 18, 2018

@husho husho referenced this pull request Jun 18, 2018

Merged

Fix panic on new game #516

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