Skip to content
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

The Great Renaming #130

Open
rjwut opened this issue Mar 20, 2018 · 6 comments
Open

The Great Renaming #130

rjwut opened this issue Mar 20, 2018 · 6 comments

Comments

@rjwut
Copy link
Member

rjwut commented Mar 20, 2018

This has been mentioned here and there, but I would like to have a full discussion on the subject of packet names.

Packets have types and subtypes. The types have canonical names which are run through JamCRC to produce their type hashes. Subtypes just have numbers, not canonical names.

The idea of renaming the packets in the documentation to correspond to their canonical names has been bandied about. The principal advantage of doing so is that we achieve greater consistency in terminology when discussing packets. (Is it TitlePacket or bigMess?) However, there are also some disadvantages:

  • Some canonical packet names are misleading. For example, some valueInt packets don't conform to the "one int value" pattern. A gmText packet isn't always sent by the GM as of v2.6.0.
  • Some packet types have names that indicate the intended form of their content rather than their purpose (valueFloat, valueInt, valueFourInts). These names are less useful, especially since as mentioned in the previous point, quite a few of them do not actually match the intended form. A valueInt packet can do things as varied as selecting a ship or console, set warp factor, set the main screen view, raise/lower shields, request docking, fire weapons, set auto DAMCON, scan targets, send keystrokes, activate upgrades, launch fighters... the list goes on and on.

Write in the comments what you think about the prospect of renaming packets.

@NoseyNick
Copy link
Collaborator

A "gmText" sent by Comms is a LITTLE misleading, but not TOO bad.

Generally speaking all the other packets that DON'T have subtypes are even less confusing? I'd even argue "objectText" is less confusing than "Intel" considering it's not ALL intel 🤷‍♂️

All the more horrible ones in your note above (valueFloat, valueInt, valueFourInts) you're ALMOST never going to be referring to it as that directly - the canonical name will presumably be the SUBTYPE, and "valueInt" only really mentioned in passing like "subtype of valueInt"?

[at the risk of feature creep] Would be really neat if we could extract "official" names for the packet SUBTYPES out of "strings -el Artemis2.7.0/Artemis.exe" but I'll confess I've never been able to spot them myself - it's possible they may even have been optimised out by Thom's compiler, or perhaps (scary thought) there never WERE any official names for the subtypes 😮

[further feeping creaturization] "Official" names for the PARAMETERS would be nice too. Generally most OBJECT parameters probably have corresponding names in the mission-file-docs.txt. Less certain about, say, "the 3rd field in a FighterPilotPacket" or even "should it actually be called rudder, Rudder, steering, Steering, deltaAngle, or something completely different?" (I note the word "rudder" doesn't appear once in mission-file-docs.txt)

@rjwut
Copy link
Member Author

rjwut commented Mar 20, 2018

"Rudder" is an artifact of the original version of ArtClientLib that I inherited when I took over the project.

I think I like trying to stick to "official" terminology whenever possible (canonical names found in the .exe and scripting XML), but favoring logical packet types for clarity and making up names for those.

As for packet field and object property names, it's an interesting idea, although lower priority, I think.

I've taken a stab creating a definitive packet name list. For packets with no subtypes, it's just the canonical name from the .exe. For subtyped packets, I have attempted to create a name that is clear, consistent, and reasonably short. Feedback welcome.

Origin: client

activateUpgrade (valueInt:0x1c)
beamRequest
buttonClick (valueInt:0x15)
commsMessage
configBeacon (valueInt:0x22)
controlMessage
convertTorpedo (valueFourInts:0x03)
craftFire (valueInt:0x1f)
craftPilot (valueFloat:0x07)
emergencyJump (valueInt:0x20)
fireTube (valueInt:0x08)
gmText
jump (valueFloat:0x05)
keystroke (valueInt:0x14)
launchCraft (valueInt:0x1e)
loadTube (valueFourInts:0x02)
ready (valueInt:0x0f)
requestDock (valueInt:0x07)
requestGridUpdate (valueInt:0x1a)
resetCoolant (valueInt:0x18)
scan (valueInt:0x13)
selectConsole (valueInt:0x0e)
selectShip (valueInt:0x0d)
sendDamcon (valueFourInts:0x04)
setAutoDamcon (valueInt:0x0c)
setBeamFreq (valueInt:0x0b)
setCaptainTarget (valueInt:0x11)
setCoolant (valueFourInts:0x00)
setCraftSettings (valueInt:0x21)
setEnergy (valueFloat:0x04)
setGmLocation (valueFloat:0x06)
setGmTarget (valueInt:0x12)
setImpulse (valueFloat:0x00)
setMainScreen (valueInt:0x01)
setPitch (valueFloat:0x02)
setSciTarget (valueInt:0x10)
setShipSettings (valueInt:0x17)
setSteering (valueFloat:0x01)
setTrim (valueInt:0x1d)
setWarp (valueInt:0x00)
setWeapTarget (valueInt:0x02)
shieldsDown (valueInt:0x06)
shieldsUp (valueInt:0x05)
toggleAutoBeams (valueInt:0x03)
togglePerspective (valueInt:0x1b)
toggleRedAlert (valueInt:0x0a)
toggleReverse (valueInt:0x19)
toggleShields (valueInt:0x04)
unloadTube (valueInt:0x09)

Origin: server

attack
autoDamcon (simpleEvent:0x0b)
bigMess
carrierRecord
clientConsoles
cloak (simpleEvent:0x07)
commsButton
commText
connected
craftLaunched (simpleEvent:0x17)
craftShake (simpleEvent:0x18)
dmx (simpleEvent:0x10)
docked (simpleEvent:0x1a)
endGame (simpleEvent:0x06)
explosion (simpleEvent:0x00)
gameOver (simpleEvent:0x1e)
gameOverReason (simpleEvent:0x14)
gameOverStats (simpleEvent:0x15)
gmButton
heartbeat
idleText
incomingMessage
jumpBegin (simpleEvent:0x0c)
jumpEnd (simpleEvent:0x0d)
keystrokeCapture (simpleEvent:0x11)
klaxon (simpleEvent:0x01)
objectBitStream
objectDelete
objectText
pause (simpleEvent:0x04)
perspective (simpleEvent:0x12)
plainTextGreeting
popup (simpleEvent:0x0a)
shipSettings
shipShake (simpleEvent:0x05)
shipSystemSync
skybox (simpleEvent:0x09)
smoke (simpleEvent:0x1b)
soundEffect (simpleEvent:0x03)
startGame
tag (simpleEvent:0x1d)

@NoseyNick
Copy link
Collaborator

Hmmm... Why have I never seen controlMessage? How do you create one of those?

There's quite a lot of setBlah?
craftFire, craftPilot, launchCraft - not craftLaunch?
shieldsDown, shieldsUp, toggleShields, not shieldsToggle?
Come to think of it, could/should we get a bit more consistent on verbNoun vs nounVerb?

I'd love to know OFFICIAL names for ... any of these. About the best I could find were from controls.ini, re-working the capitalisation...

helmToggleReverse
normalizeAllCoolant
commsRedAlert
helmRequestDock
toggleShields
raiseShields
lowerShields
toggleAutoBeams
scienceScanButton
convertTorpToEnergy
convertEnergyToTorp
cycleTorpTube0
fireTorpTube0
analogClimbDive

I won't pretend I like them all... or even most of them 😕
Ah, I DO now see where the word "rudder" came from though.

About the best I could find in mission-file-docs.txt ...
create
destroy (is this the same as objectDelete packet?)
incomingMessage (check!)
bigMessage (but we know the corresponding packet is bigMess)
endMission
incomingCommsText (but we know the packet is commText and/or commsMessage)
playSoundNow (is that controlMessage? or soundEffect? or something else?)
setShipText (objectText packet?)
startGettingKeypressesFrom / endGettingKeypressesFrom (yuck)
setGmButton (gmButton packet) / setCommsButton (commsButton packet) - perhaps an argument for dropping "set" everywhere?
clearGmButton / clearCommsButton
setMonsterTagData (simpleEvent:0x1d "tag"? Lost the "set"? monsterTagData?)
setNamedObjectTagState (eww)

... and lots of attribute names we should probably adopt for object fields... EG angle/pitch/roll (NOT rudder)

@rjwut
Copy link
Member Author

rjwut commented Mar 27, 2018

@NoseyNick

So controlMessage only happens in scripted missions, when COMMs is notified that it has an audio message and COMMs responds by clicking either PLAY or DISMISS.

I would like to be more consistent with verbNoun versus nounVerb, definitely. Although some are nounAdjective (shieldsUp and shieldsDown, for example, but as you mentioned, we could use raiseShields and lowerShields instead).

The one thing I don't like about dropping "set" is it makes it unclear whether the packet is the server telling the client about a change, or the client requesting a change from the server. Without a "set" precedent, we couldn't tell, for example, whether autoDamcon is the server telling the client about an auto DAMCON update, or the client asking the server to change it.

@rjwut
Copy link
Member Author

rjwut commented Mar 27, 2018

Here are my comments on some selected names:

Names I don't like but am loathe to change because they're canonical

  • attack (s2c): There's more than one kind of attack. This is called BeamFiredPacket in the docs, which is much more explicit.
  • beamRequest (c2s): Okay, yeah, technically the client is requesting that the server fire a beam weapon. But this isn't like requestDock. Why isn't this just something like fireBeam?
  • bigMess (s2c): Okay, yeah, it's funny, but it's not obvious to someone unfamiliar with it what this is about.
  • controlMessage (c2s): This ought to be something like audioAction or something else that makes it more clear what we're talking about here.
  • objectDelete (s2c): This violates the verbNoun pattern; deleteObject would be preferred.
  • shipSystemSync (s2c): To conform to the verbNoun pattern, it should be syncShipSystems.

Non-canonical names that I agree we should change

  • buttonClick (valueInt:0x15) (c2s): Yeah, this should be clickButton.
  • jumpBegin and jumpEnd (simpleEvent:0x0c/0d) (s2c): Change to beginJump and endJump.
  • keystrokeCapture (simpleEvent:0x11): Change to captureKeystroke.
  • shieldsUp and shieldsDown (valueInt:0x05/06): I agree, make these raiseShields and lowerShields.

Non-canonical names that I'm not sure about

  • autoDamcon (simpleEvent:0x0b): See the "set" discussion below.
  • craftFire (valueInt:0x1f) (c2s): Doesn't follow verbNoun, but fireCraft makes it seem like the craft is the projectile, and fireFromCraft feels clunky.
  • craftPilot (valueFloat:0x07) (c2s): Same.
  • setAutoDamcon (valueInt:0x0c) (c2s): You're suggesting dropping "set", but that would make it indistinguishable from autoDamcon (simpleEvent:0x0b) (s2c). And toggleAutoDamcon would be inaccurate because it actually specifies the desired the auto DAMCON state.
  • set* (c2s): Most of these could have "set" dropped without causing too much ambiguity, since most of the settings they control are updated by objectBitStream. But I would want to be consistent, so it depends what we'd do with the autoDamcon issue.
  • setTrim (c2s): We need some good way to distinguish between the packet which sets the desired pitch versus the "just go up/down" one. The analogClimbDive name does seem to be one way to handle that, but I don't really like the ClimbDive term when there are more concise terms such as "pitch" or "trim" or even "attitude".
  • shipShake (simpleEvent:0x05): To conform to the verbNoun pattern, this ought to be shakeShip, but "shipshake" is a somewhat well-known term for what happens on shows like Star Trek when weapons hit the ship. (By the way, check out some fun stabilized shipshake GIFs for a laugh.

@StarryWisdom
Copy link
Collaborator

beamRequest is actually a great example of one I'm unsure of I think there wants to be consistency here, presumably the server checks if the ship can fire and thus the "request"

now most the time the answer is probably yes so the question becomes should all the actions that require response from the server have a request in front of them?, even if normally players are unaware of them?

in my code I had been using internal messages split on the "requestBeam/fireBeam" line

though come to think of it maybe by supporting the official names we already have inconstancy here anyway?

other thoughts

do we have a proper name anywhere for if its an emergency jump or a combat jump - I have heard both from players.

popup may want to be called warningPopup to match scripting

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants