Added functions for VectorLines and fleshed out the SnapPoints functions #2
Conversation
There was a problem hiding this comment.
Thanks for this. There's a few minor things I'd appreciate if you could adjust.
Beyond the inline comments, if you're bothered, my preference for unions is now to put spaces around the vertical bar. You'll find that through-out this API that's not the case, but if you can be bothered putting space in that'd be great - I won't hold up the PR if you don't though.
Another thing, technically tts__SnapPoint should actually be two types i.e. tts__SnapPoint and (I guess) tts__SnapPointParameters; not dead set on the name. The difference is that tts__SnapPointParameters would be used in the setters and have optional (nil |) fields, and the tts__Vector should be tts__VectorShape.
However, again, I won't let that hold up the PR, it was already this way and I'm already roping you into fixing things you didn't write. So just if you can be bothered.
tts/Object.def.lua
Outdated
| --- Returns a table of sub-tables, each sub-table representing VectorLineGroup. | ||
| ---@return tts__VectorLineGroup[] | ||
| function Object.getVectorLines() | ||
| return {} |
There was a problem hiding this comment.
If you could please omit the return statements that'd be appreciated.
Instead you'll want to disable the return inspection for .def.lua files:
Benjamin-Dobell/IntelliJ-Luanalysis#7
There was a problem hiding this comment.
Fixed, but I will tell you that document is currently incorrect.I had to use the pattern file:*.def.lua without the **/
There was a problem hiding this comment.
May be because you're using source roots where as I'm using a sub-directory. If you want to add a comment about that on the issue so that others may benefit, that'd be appreciated.
tts/global.def.lua
Outdated
| ---@field position tts__Vector Position of the snap point. The position is relative to the entity's center (a local position). | ||
| ---@field rotation tts__Vector Rotation of the snap point. The rotation is relative to the entity's rotation (a local rotation). | ||
| ---@field rotation_snap boolean If the snap point is a "rotation" snap point. |
There was a problem hiding this comment.
I know it wasn't the case, and that you've just copied and paste from another file. However for consistency, could you please start the comments (post-type) with @.
tts/global.def.lua
Outdated
|
|
||
|
|
||
| ---@shape tts__VectorLineGroup | ||
| ---@field points tts__Vector[] An array of 2 or more points. Lines are drawn between each consecutive pair of points. |
There was a problem hiding this comment.
As per the above, could you please add a @ prefix to the comment?
There's also some errant double spaces in the comment and also for rotation (2 lines below). That's minor enough that I'd normally let it slide, but seems as I'm requesting other changes, may as well throw it in.
| @@ -358,9 +358,6 @@ function Object.setPosition(position) end | |||
| ---@return true | |||
| function Object.setPositionSmooth(position, collide, fast) end | |||
|
|
|||
| --- Spawns snap points from a list of parameters. | |||
| ---@param parameters tts__Object_SnapPoint[] | |||
| function Object.setSnapPoints(parameters) end | |||
There was a problem hiding this comment.
Could you please restore the alphabetical order?
For working code I understand that sorting things alphabetically can be a bit of a fool's game, as refactoring breaks the whole thing. However, seems as these files are purely documentation, I think there's value to the consistency.
Grouping functions by their relation to each other certainly has merit (and is how I tend to write code). However, it is subjective, and since we're not author's of the API in question we don't really have the authority to say which methods are most related to which other methods.
There was a problem hiding this comment.
For the record there are other functions not in alphabetical order, so it was not clear that that was the spec. Done
There was a problem hiding this comment.
Understood.
This file ought to be alphabetical, however other files definitely aren't. Particularly Vector and Color as their types were built by literally copy and paste from https://github.com/Berserk-Games/Tabletop-Simulator-Lua-Classes/
tts/global.def.lua
Outdated
| --- Returns a table of sub-tables, each sub-table representing VectorLineGroup. | ||
| ---@return tts__VectorLineGroup[] | ||
| function getVectorLines() | ||
| return {} |
There was a problem hiding this comment.
As above, could you please omit the return statement and move the definition to retain alphabetical ordering?
| ---@overload fun(parameters: table):string | ||
| ---@param parameters tts__SnapPoint[] | ||
| ---@return string | ||
| function setSnapPoints(parameters) end |
There was a problem hiding this comment.
Looks like it was already this way. However, could we rename parameters to snapPoints?
|
|
||
| --- Render vector lines from a list of parameters. | ||
| ---@param parameters tts__VectorLineGroup[] | ||
| function setVectorLines(parameters) end |
There was a problem hiding this comment.
Maybe "groups" instead of "parameters"?
tts/Object.def.lua
Outdated
| ---@param parameters tts__VectorLineGroup[] | ||
| function Object.setVectorLines(parameters) end | ||
|
|
||
| --- Returns a table of sub-tables, each sub-table representing VectorLineGroup. |
There was a problem hiding this comment.
What about?
Returns an array of vector line groups.
I know that the official TTS docs are devoid of the word "array", however seems as these are types for Luanalysis, which has a concept of arrays, I think we can take some creative liberty 😛
tts/global.def.lua
Outdated
| ---@return string | ||
| function setSnapPoints(parameters) end | ||
|
|
||
| --- Returns a table of sub-tables, each sub-table representing one snap point. |
There was a problem hiding this comment.
As above, preference would be to use the term array, where it makes sense to do so.
tts/global.def.lua
Outdated
| ---@param parameters tts__VectorLineGroup[] | ||
| function setVectorLines(parameters) end | ||
|
|
||
| --- Returns a table of sub-tables, each sub-table representing VectorLineGroup. |
There was a problem hiding this comment.
As above, "array" is preferable.
tts/Object.def.lua
Outdated
| ---@return tts__Object_SnapPoint[] | ||
| function Object.getSnapPoints() end | ||
| ---@return tts__SnapPoint[] | ||
| function Object.getSnapPoints() return {} end |
There was a problem hiding this comment.
Missed this return statement in my initial review. Just a reminder it's here.
There was a problem hiding this comment.
Sorry, I missed this in the initial review and think our messages/commits cross. If you could remove this return statement, that'd be appreciated.
dcorbin
force-pushed
the
snapPoints_and_vectorLines
branch
2 times, most recently
from
a9004ec
to
b7d4281
Compare
|
I think I've addressed all the concerns in all the contexts, other than alphabetizing functions I didn't touch. |
dcorbin
force-pushed
the
snapPoints_and_vectorLines
branch
from
b7d4281
to
8441fde
Compare
|
Thanks for PR, and quickly following up on those change requests. Very much appreciated! |
No description provided.