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

feat: add RoomTerrain.getRawBuffer() #247

Merged
merged 4 commits into from
Dec 2, 2023

Conversation

admon84
Copy link
Member

@admon84 admon84 commented Sep 20, 2023

Brief Description

Adds type and test for this method:
https://docs.screeps.com/api/#Room.Terrain.getRawBuffer

Checklists

  • Test passed
  • Coding style (indentation, etc)
  • Edits have been made to src/ files not index.d.ts
  • Run npm run dtslint to update index.d.ts

@DiamondMofeng DiamondMofeng self-requested a review September 21, 2023 08:43
package.json Outdated
@@ -1,6 +1,6 @@
{
"name": "typed-screeps",
"version": "3.3.3",
"version": "3.3.4",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does not need to bump version here

* @param destinationArray (optional) A typed array view in which terrain will be copied to.
* @return Uint8Array Copy of underlying room terrain as a new Uint8Array typed array of size 2500.
*/
getRawBuffer(destinationArray?: Uint8Array): Uint8Array;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type of destinationArray cannot only be Uint8Array. Any TypedArray is fine. The source code in the screeps engine uses TypedArray.prototype.set to copy data into destinationArray.
(see https://github.com/screeps/engine/blob/master/src/game/rooms.js#L398 and https://github.com/screeps/engine/blob/master/src/game/rooms.js#L1237)

Btw, it's okay to copy value from uint8array to uint16array. The reverse also works

const terrainData = new Uint8Array([1, 2, 3, 4, 5]);

const uint8Receiver = new Uint8Array(5);
uint8Receiver.set(terrainData, 0); // okey
console.log(uint8Receiver); // [1, 2, 3, 4, 5]

const uint16Receiver = new Uint16Array(5);
uint16Receiver.set(terrainData, 0); //okay
console.log(uint16Receiver); // [1, 2, 3, 4, 5]

const normalReceiver = new Array(5);
Uint16Array.prototype.set.call(normalReceiver, terrainData); // error: this is not a typed array.

from uintArray16 to uintArray8

const terrainData = new Uint16Array([256,888]);

const uint8Receiver = new Uint8Array(5);
uint8Receiver.set(terrainData, 0); // okey
// (256 & ((1<<8)-1)) is 0, (888 & ((1<<8)-1)) is 120
console.log(uint8Receiver); // [ 0, 120, 0, 0, 0 ] 

const uint16Receiver = new Uint16Array(5);
uint16Receiver.set(terrainData, 0); //okay
console.log(uint16Receiver); // [ 256, 888, 0, 0, 0 ]

Copy link
Member

@DiamondMofeng DiamondMofeng Sep 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should define a type alias of the underlying representation, then use it as the default type parameter.

Here are my suggestions for typing this method.
The first seems to be less correct. The second uses overloads, but I think it fits better.

getRawBuffer<T extends TypedArray = Uint8Array>(destinationArray?: T): T;

or

getRawBuffer<T extends TypedArray>(destinationArray: T): T;
getRawBuffer(): Uint8Array;

In case there is no built-in union for TypedArray (Big(U)Int64Array is not compatible):

type TypedArray = Int8Array | Uint8Array | Int16Array | Uint16Array | Int32Array | Uint32Array | Uint8ClampedArray | Float32Array | Float64Array;

/**
* Get copy of underlying static terrain buffer.
* @param destinationArray (optional) A typed array view in which terrain will be copied to.
* @return Uint8Array Copy of underlying room terrain as a new Uint8Array typed array of size 2500.
Copy link
Member

@DiamondMofeng DiamondMofeng Sep 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although the documentation says that this method may return ERR_INVALID_ARGS, I do not see it in the source of screeps engine. So it's okay to ignore it. Anyway, maybe we should leave a comment about it here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, how about adding a jsdoc comment about possible error here

* @throws {RangeError} if `destinationArray` is provided, it must have a length of at least 2500 (`50*50`).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the in-depth review. I'll update the branch tomorrow and let you know when it's ready for another review.

Copy link
Member

@DiamondMofeng DiamondMofeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your work❤️.
Maybe it's better to make the TypedArray inline, considering that our types are globally accessible. I'll change it and merge this pr later.

@admon84
Copy link
Member Author

admon84 commented Sep 23, 2023

I moved the TypedArray to inline and ran the npm scripts for formatting/compile/lint/etc. Feel free to make any other adjustments though!

@DiamondMofeng DiamondMofeng merged commit 9d727de into screepers:master Dec 2, 2023
3 checks passed
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

Successfully merging this pull request may close these issues.

2 participants