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

Implemented blip opcodes #436

Merged
merged 1 commit into from May 11, 2018

Conversation

Projects
None yet
4 participants
@dracc
Copy link
Contributor

dracc commented May 3, 2018

As written by @husho in #409

@dracc dracc force-pushed the dracc:blip-409 branch from c33de4f to 738389f May 3, 2018

data.type = BlipData::Instance;
data.colour = 6; // @todo 4 in Vice City
case BlipData::Coord:
data.colour = 5;

This comment has been minimized.

@JayFoxRox

JayFoxRox May 3, 2018

Collaborator

nit: The Vice City comments were usefull in my opinion.

//Edit: I just realized there are some comments about this below. I still don't understand this change, so maybe this comment can be ignored.

This comment has been minimized.

@husho

husho May 4, 2018

Contributor

Ugh, it was a long time ago, so I don't really remember what was the purpose of this change. BUT I think there was missing bilp type. Although, these values are from original game.

This comment has been minimized.

@dracc

dracc May 6, 2018

Contributor

It seems a function was added and the "deleted" values have been revised and git has, instead of being easy-to-read chosen to be efficient and reuse as many lines as possible. Looks a mess, but if one reads more code it stands clear that nothing was really lost, just re-organized. :)

@arg arg2
@arg arg3
@arg blip Blip
opcode 0162 / 354

This comment has been minimized.

@JayFoxRox

JayFoxRox May 3, 2018

Collaborator

The decimal is very confusing here

}

/**
@brief %4d% = create_marker_above_actor %1d% color %2d% visibility %3d%
@brief ADD_BLIP_FOR_CHAR_OLD pedHandle colour display outblipHandle

This comment has been minimized.

@JayFoxRox

JayFoxRox May 3, 2018

Collaborator

Is this change really useful? The previous comment has been auto-generated. This change is only fine if we did this renaming for other functions too.

However, I'm also worried this might even be the R* function name, which we tried to avoid for legal reasons.

This comment has been minimized.

@husho

husho May 4, 2018

Contributor

In fact, it is original R* function name.

@arg pedHandle Ped
@arg colour Blip colour
@arg display Blip display mode
@arg outblipHandle Created blip

This comment has been minimized.

@JayFoxRox

JayFoxRox May 3, 2018

Collaborator

The @arg was previously easily parsable by a computer. Now it's not. Check other instances and how we handled this in the past.

This comment has been minimized.

@dracc

dracc May 5, 2018

Contributor

I'm not sure I follow.
Was the name "argX" (where X is a number) easier to parse, or what is the actual difference?

This comment has been minimized.

@JayFoxRox

JayFoxRox May 5, 2018

Collaborator

Take my commnt as an example on how not to write a review comment. I don't know what I meant myself.
But I think it has to do with the actual argument being called blip when this one refers to outblipHandle

/**
@brief ADD_BLIP_FOR_OBJECT_OLD objectHandle colour display outblipHandle
opcode 0163 / 355

This comment has been minimized.

@JayFoxRox

JayFoxRox May 3, 2018

Collaborator

For this comment block, see other instance above.
Also take care of other instances which follow.

@@ -11717,8 +11716,8 @@ void opcode_03dc(const ScriptArguments& args, const ScriptPickup pickup, ScriptB
@arg arg2
@arg blip
*/
void opcode_03dd(const ScriptArguments& args, const ScriptPickup pickup, const ScriptRadarSprite arg2, ScriptBlip& blip) {
auto data = script::createObjectBlipSprite(args, pickup, arg2);
void opcode_03dd(const ScriptArguments& args, const ScriptPickup pickup, const ScriptRadarSprite blipSprite, ScriptBlip& blip) {

This comment has been minimized.

@JayFoxRox

JayFoxRox May 3, 2018

Collaborator

Argument renamed but unchanged in comment. Please review other instances too.

@dracc

This comment has been minimized.

Copy link
Contributor

dracc commented May 4, 2018

Great input @JayFoxRox and @husho. Thanks!
I'll look into it ASAP.

Implemented blip opcodes as per #409
Renamed some functions and updated comments accordingly.

@dracc dracc force-pushed the dracc:blip-409 branch from 738389f to 36e73ab May 6, 2018

@dracc

This comment has been minimized.

Copy link
Contributor

dracc commented May 6, 2018

Comments have been addressed and taken care of.

@danhedron danhedron merged commit 9831314 into rwengine:master May 11, 2018

2 checks passed

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

@dracc dracc deleted the dracc:blip-409 branch May 24, 2018

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