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] Implemented some locate opcodes #438

Merged
merged 1 commit into from May 14, 2018

Conversation

Projects
None yet
4 participants
@dracc
Copy link
Contributor

dracc commented May 3, 2018

As written by @husho in #408

@dracc

This comment has been minimized.

Copy link
Contributor

dracc commented May 6, 2018

Placeholders for ScriptVec3, would that (if it's the third argument) be %3d% or %3o%?

@husho

This comment has been minimized.

Copy link
Contributor

husho commented May 6, 2018

neither, o is for models and d is for numbers

@dracc

This comment has been minimized.

Copy link
Contributor

dracc commented May 6, 2018

That sounds sane. But what would be the right one?

@husho

This comment has been minimized.

Copy link
Contributor

husho commented May 6, 2018

v2, v3? but then positions would misalign

@dracc

This comment has been minimized.

Copy link
Contributor

dracc commented May 6, 2018

@JayFoxRox clarified on irc that the placeholders are generated, so I will leave them be (mostly).

@JayFoxRox

This comment has been minimized.

Copy link
Collaborator

JayFoxRox commented May 6, 2018

I think you misunderstood. So I'll answer a bit more detailed:

Not the placeholder is generated, but the entire "GTA3ModuleImpl.inl".

The @brief lines, including the placeholders come from SCM.ini - that is a community created file and used for SCM assemblers. So instead of fixing this stuff in OpenRW, it should be fixed in the community documentation (which is turned into code). Using the SCM.ini we can generate a patch for the comments in our file (using the same script we used to create the very first "GTA3ModuleImpl.inl").

The text between the placeholders is whatever you feel like - but in our case it should be consistent with the names used by the GTA modding community.
The placeholders themselves also follow a logical scheme and are computer parsable as they are used in SCM assemblers - there is no debate how they are, there is only one correct way.
Changing them would break SCM assemblers if someone would update SCM.ini with wrong information from OpenRW!

However, this code in OpenRW is the interface from this assembly like SCM language (with mostly scalar float and integer arguments) to a high-level language (which also supports structures / vectors). That's why the @brief might list 6 arguments (as required for SCM assembly) but our functions only take 3 (because the scalars are combined into structures).

In our case, the @brief is included so you don't have to look up every SCM command manually before knowing what it does. But ideally, if you are familar with SCM, you know that exact wording from the SCM assemblers. Using existing naming conventions also avoids ambiguity and makes it easy to find information using google or in disassembled missionscripts.
You should not look at it to count the number of arguments in OpenRW or anything. It's to tell about meaning and make our source code searchable, not to inform about specific details. Consult more complete SCM documentation elsewhere for that.

Also, only if we don't modify the @brief manually we'll be able to generate proper patches whenever the community docs update (or if we want to switch to another naming scheme). Changing the @brief manually causes a ton of merge conflicts, so you should even avoid whitespace changes.

@dracc

This comment has been minimized.

Copy link
Contributor

dracc commented May 7, 2018

So, instead of "mostly", it would be preferable to not touch them at all?
What about the @arg lines, are those only related to OpenRW?
Great write-up, thank you @JayFoxRox!

@dracc dracc referenced this pull request May 7, 2018

Closed

Implemented various opcodes. #435

@dracc dracc force-pushed the dracc:locate-408 branch from 146238d to 263f861 May 7, 2018

@JayFoxRox

This comment has been minimized.

Copy link
Collaborator

JayFoxRox commented May 7, 2018

Exactly: Don't touch.
Yes, @arg lines are specific documentation for OpenRW. They are also generated, but they describe our arguments (and we can leave them out of future patches when re-generating documentation).

Also the argument types ScriptInt / ScriptFloat / ... are also generated from another community based file: gta3sc's XML (which, however, is almost always correct as it's based on R* files). So they should also be fixed upstream when we change something in ours.

@dracc dracc changed the title Implemented some locate opcodes [Ready] Implemented some locate opcodes May 14, 2018

@dracc

This comment has been minimized.

Copy link
Contributor

dracc commented May 14, 2018

@brief-lines untouched, @arg-lines given correct argument names.
I believe this is ready to merge.

@danhedron danhedron merged commit 274fe20 into rwengine:master May 14, 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:locate-408 branch May 24, 2018

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