Javascript function to get cellId from map point

Question

I wrote this, and I would like to know if this code is clean.

It lets me know if a point is in the middle of an isometric cell.

const getMapPointFromCellId = cellId => {
 var {x, y} = getCoordinatesFromCellId(cellId)
 x = x * constants.cellWidth
 if (y % 2) x += constants.offsetWidth
 y = y * constants.offsetHeight 
 return {x, y}
}
const getCellIdFromMapPoint = (x, y) => {
 var cellId = 0
 while(cellId < constants.numberCells){
 const cellCoordinates = getMapPointFromCellId(cellId)
 const offsetX = cellCoordinates.x + constants.offsetWidth
 const offsetY = cellCoordinates.y + constants.offsetHeight
 const endX = cellCoordinates.x + constants.cellWidth
 const endY = cellCoordinates.y + constants.cellHeight
 if (cellCoordinates.x < x && x < cellCoordinates.x + constants.cellWidth)
 if (cellCoordinates.y < y && y < cellCoordinates.y + constants.cellHeight){
 if (x < offsetX && y < offsetY) 
 if ((offsetY - y) / (cellCoordinates.x - x) > (offsetY - cellCoordinates.y) / (cellCoordinates.x - offsetX)) return cellId
 if (x > offsetX && y < offsetY)
 if ((offsetY - y) / (endX - x) <= (offsetY - cellCoordinates.y) / (endX - offsetX)) return cellId
 if (x < offsetX && y > offsetY) 
 if ((offsetY - y) / (cellCoordinates.x - x) <= (offsetY - endY) / (cellCoordinates.x - offsetX)) return cellId
 if (x > offsetX && y > offsetY) 
 if((offsetY - y) / (endX - x) >= (offsetY - (constants.cellHeight - constants.offsetHeight)) / (endX - offsetX)) return cellId
 }
 cellId++
 }
 return -1
}

• le_m: why the hate for for-loops?
• ken: @le_m the hate for for-loops? I just like while ^^
• Sᴀᴍ Onᴇᴌᴀ: Welcome to Code Review! I changed the title so that it describes what the code does per site goals: "State what your code does in your title, not your main concerns about it.". Feel free to give it a different title if there is something more appropriate. Also, can you please include the code for getMapPointFromCellId? until then, this post is basically off-topic
• ken: @SamOnela Thank you. Sorry for errors, I have added the missing code

Answer 1

General javascript style notes

• Clean javascript has semicolons in the right places.

// bad
const foo = 1
// good
const foo = 1;

• Clean Javascript always uses delimited blocks

// worst
if(foo === bar) poo = 1
// bad
if(foo === bar) poo = 1;
// good
if (foo === bar) {
 poo = 1;
}
// or
if (foo === bar) { poo = 1; }
// or to satisfy some linters semicolons only at line end
if (foo === bar) { poo = 1 }

• Clean Javascript does not repeat itself or have superfluous content

Bad

 const endX = cellCoordinates.x + constants.cellWidth
 const endY = cellCoordinates.y + constants.cellHeight
 if (cellCoordinates.x < x && x < cellCoordinates.x + constants.cellWidth)
 if (cellCoordinates.y < y && y < cellCoordinates.y + constants.cellHeight){

Good

 const endX = cellCoordinates.x + constants.cellWidth
 const endY = cellCoordinates.y + constants.cellHeight 
 // only one if statment needed
 if (cellCoordinates.x < x && x < endX && cellCoordinates.y < y && y < endY){

• General rule is no calculations inside statements

// bad
if((offsetY - y) / (endX - x) >= (offsetY - (constants.cellHeight - constants.offsetHeight)) / (endX - offsetX)) return cellId
// good
const ySlope = (offsetY - y) / (endX - x);
const yOffsetSlope = 
 (offsetY - (constants.cellHeight - constants.offsetHeight)) /
 (endX - offsetX);
if (ySlope >= yOffsetSlope) {
 return cellId;
}

More points

It is nice to have descriptive names, it is not nice when descriptive names get in the way of code readability. It is even worse when names are in conflict.

// a map point is a cell coordinate???
const cellCoordinates = getMapPointFromCellId(cellId)
// but the function called gets the cell coordinates
const getMapPointFromCellId = cellId => {
 var {x, y} = getCoordinatesFromCellId(cellId)
 x = x * constants.cellWidth
 if (y % 2) x += constants.offsetWidth
 y = y * constants.offsetHeight 
 return {x, y}
}

Should the variable not be const cellMapPoint = getMapPointFromCellId(cellId)

And constants is meaningless out of context.

And speaking of context you prefixe some variable names with cell and others not.

// prefixed
var cellId = 0 
while(cellId < constants.numberCells){
 // prefixed
 const cellCoordinates = getMapPointFromCellId(cellId)
 // not prefixed
 const offsetX = cellCoordinates.x + constants.offsetWidth
 const offsetY = cellCoordinates.y + constants.offsetHeight
 // not prefixed
 const endX = cellCoordinates.x + constants.cellWidth
 const endY = cellCoordinates.y + constants.cellHeight

It is clear that the function is dealing with cells and that you don't need to prefix some names so why prefix others??

And from the code I can not workout if you are using an Id or an index. An id is different than an index and you would not test an id against a count while(cellId < constants.numberCells){ nor would you increment an id without testing if it exists so I think its an index.

const getCellIndexFromMapPoint = (x, y) => {
 var index = 0;
 const cells = constants; // ALIAS
 while(index < constants.numberCells){
 const mapPoint = getMapPointFromCellIndex(index);
 const offsetX = mapPoint.x + cells.offsetWidth;
 const offsetY = mapPoint.y + cells.offsetHeight;
 const endX = mapPoint.x + cells.cellWidth;
 const endY = mapPoint.y + cells.cellHeight;
 if (mapPoint.x < x && x < endX && mapPoint.y < y && y < endY){
 // and so on
 }
 index += 1;
 }

Axonometric projections

With all that said the whole function is a problem as is doing a search when a direct index can be found.

_{* note that the code below is off the top of my head and may contain typos, it is a suggestion and you should do some extra research on axonometric projections to create code more suited to your needs.*}

All axonometric projections (Isometric is a particular type of axonometric projection) can be defined as 4 2d vectors that define the direction and scale of each of the 3D axis X,Y,Z and the location of the origin. If we ignore the Z as we are after a cell coordinate the problem can be simplified even further.

enter image description here

Consider the image above, The x axis and y axis are defined as a 2D vector each with an X and y magnitude that usually defines the size of a pixel. The origin defines where in screen space the coordinate 0,0 will appear.

You can define the axis as a matrix

const projection = [
 1, 0.5, // direction and size of x axis
 -1, 0.5, // direction and size of y axis
 0, 0 // the origin
];

From that matrix you can calculate a screen position for a pixel

function getScreenPos(x, y, screenPos = {}) {
 const P = projection; // alias
 screenPos.x = x * P[0] + y * P[2] + P[4];
 screenPos.y = x * P[1] + y * P[3] + P[5];
 return screenPos;
}

Create a projection

In that projection matrix is all you need to convert from screen space to world space (the reverse of the above function). So when you define the projection matrix you also define the inverse matrix

function defineProjection(origin, xAxis, yAxis){
 const m = [ // the projection matrix
 xAxis.x, xAxis.y,
 yAxis.x, yAxis.y,
 origin.x, origin.y,
 ];
 const im = []; // the inverse matrix
 // get the determinate (I call it cross as it is a cross product of two vectors)
 const cross = m[0] * m[3] - m[1] * m[2];
 im[0] = m[3] / cross;
 im[1] = -m[1] / cross;
 im[2] = -m[2] / cross;
 im[3] = m[0] / cross; 
 // define the projection matrix object
 return {
 matrix : m,
 invMatrix : im,
 toScreen(worldPos, screenPos = {}) {
 const W = worldPos; // alias
 screenPos.x = W.x * m[0] + W.y * m[2] + m[4];
 screenPos.y = W.x * m[1] + W.y * m[3] + m[5];
 return screenPos;
 },
 toWorld(screenPos, worldPos = {}) {
 const x = screenPos.x - m[4];
 const y = screenPos.y - m[5];
 worldPos.x = x * im[0] + y * im[2];
 worldPos.y = x * im[1] + y * im[3];
 return worldPos;
 }, 
 };
 } 

Create a map.

Now if you have a regular grid of cells that is defined as an array with a modulo. mapDesc defines the number of cells across and down. cellDesc defines the size of each cell. items are the cells as an array where a cell's position is related to the array index

var worldPos = {
 x : (index % cells.mapDesc.width) * cells.cellDesc.width,
 y : (index / cells.mapDesc.width | 0) * cells.cellDesc.height
}
// and screen pos is
var screenPos = cells.projection.toScreen(worldPos);

The object below is a bare bones simple version

 const cells = {
 items : [], // contains cells
 projection : defineProjection( 
 { x : 1, y : 0.5 }, // x axis
 { x : -0.5, y : 1 }, // y axis
 { x : 0, y : 0 }, // origin
 ),
 mapDesc : {
 width : 32, // number cells 
 height : 32,
 },
 cellDesc : {
 width : 64,
 height : 64,
 },
 getCellAt(screenPos) {
 const worldPos = cells.projection.toWorld(screenPos);
 worldPos.x /= cellDesc.x;
 worldPos.y /= cellDesc.y;
 worldPos.x |= 0; // floor
 worldPos.y |= 0; // floor
 if (worldPos.x >= 0 && worldPos.x < cells.mapDesc.width &&
 worldPos.y >= 0 && worldPos.y < cells.mapDesc.height) {
 const cellIndex = worldPos.x + worldPos.y * mapDesc.width;
 return cells.items[cellIndex];
 }
 },
 }

The above object defines an array of cells (items) and a description of the projection used and details about a cell size and details about the cell layout.

You can call the function getCellAt({x : ?, y : ?}) and it returns a cell if you have given a coordinate over a cell. It does it very fast compared to a search.

Improved search

Even if the cells are randomly placed and you need to do a search, converting to world coordinates means you can work in axis aligned coordinates rather than the complex inversion you do for each cell.

// your search function need only do
const worldPos = projection.toWorld({x,y});
const endX = cell.x + constants.cellWidth
const endY = cell.y + constants.cellHeight
if (cell.x < worldPos.x && worldPos.x < endX && 
cell.y < worldPos.y && worldPos.y < endY){ 
 return index;
}

• ken: Your solution seems much more adapter, but I do not understand it yet; I will need time. I corrected the first code you provided me, and thanks. But keeping this first function is correct. I do not understand why it's a problem
• Blindman67: @ken I don't know how the rest of your code is structured, but doing a search to find a map (world) location is slow. I provided a method of converting between map (world) coordinates and screen coordinates and back to make it a lot easier to test if a point is over a cell.
• ken: I think it's not slow enough to realize slowness compared to your code? No?
• Blindman67: @ken It depends on the number of cells, how often you are searching and how they are arranged, all of which I don't know.

Answer 2

I always like to document my functions using high-level doc-blocks, e.g.

/**
 * Return the ID of the cell whose center is at the given point or -1 if ...
 *
 * @param {number} x - The x coordinate of the given point.
 * @param {number} y - The y coordinate of the given point.
 * @return {number} cell ID or -1 if no cell found at given point.
 */
const getCellIdFromMapPoint = (x, y) => { ... }

Also, I use inline comments to help the reader and my future self to understand complex statements such as

// Do C if A and B or C:
if (x < offsetX && y < offsetY) 
 if ((offsetY - y) / (cellCoordinates.x - x) > (offsetY - cellCoordinates.y) / (cellCoordinates.x - offsetX)) return cellId

Better yet, take those complex statements as a hint to better refactor your code.

I advise to use available language constructs where appropriate. If you replace a simple for-loop with a while-loop for no apparent reason, a fellow developer will have a harder time to read and understand your code.

Also, I really recommend to use curly brackets with if-statements. The lack of those might be more pleasing to the eye by hiding the deep nesting, but makes reading and understanding as well as modifying your code much harder.

I suggest to combine if-clauses with identical statements. Instead of writing e.g.

if (A)
 if (B)
 return cellId;
if (C)
 if (D)
 return cellId

write e.g.

if ((A && B) || (C && D)) {
 return cellId;
}

Better yet, write helper functions to reduce the complex conditions, e.g.

if (isSomeCondition(A, B) || isSomeOtherCondition(C, D)) {
 return cellId;
}

Give those functions clear, self-documenting names to help the reader along. In your case, those could be e.g. isWithinRect(x, y, rect) or similar.

Your algorithm's runtime complexity is probably linear in the number of available cells. If you find that your application is running slow because of that, you can employ techniques such as spatial partitioning to improve runtime performance.

• I wrestled a bear once.: Nitpicks... If you're going to document functions you should use the proper syntax so your IDE can understand your functions. While loops, while less concise, are still just as common as for loops. If a dev can't understand a while loop they shouldn't be maintaining anybody's code. Other than that, great suggestions.
• le_m: @Iwrestledabearonce. Good advice regarding professional documentation. AFAIK the given syntax is proper though, so the comment should already pop up in an IDE / be included in autogenerated docs. Regarding while-loops: There are of course good use-cases for those, but here OP initializes a counter, loops till the counter reaches a certain limit and increments within the loop body - that's basically what for-loops have been made for. Seeing a for-loop there with all those loop counter related statements in one single place would have speed up my understanding of the code.
• I wrestled a bear once.: ok, fair enough. regarding commenting tho, the IDE will read your entire statement as a description rather than actually documenting the params and return types. i defend while loops because i frequently find myself in the opposite position, writing loops like for(;someCondition;) ...ignoring the first and the last statements when i really should be using a while loop instead.
• le_m: @Iwrestledabearonce. Yep, those teary eyed for (;;;) are equally bad... I included some more annotations so OP has a better impression of the power of documenting comments.
• Blindman67: Though not your fault this does illustrate the problem with documentation. MapPoint as far as I can tell is a world coordinate, thus the documentation should clearly define that it is not just a coordinate, and it is completely unclear in the code as to what ID is, from the look of the code it is an index not an ID. Documentation should show the abstracted data meaning, and if not available it should not make guesses (again not your fault just pointing out the dangers).

Answer 3

• Combine your conditionals. especially this first one:

  var cellWidthFits = cellCoordinates.x < x && x < cellCoordinates.x + constants.cellWidth;
  var cellHeightFits = cellCoordinates.y < y && y < cellCoordinates.y + constants.cellHeight
  if(cellWidthFits && cellHeightFits)
  

• If you can't fit the entire if statemtment on a single line, please use the curly brackets. Like the semicolons, they're not required, but your leaving yourself open to bugs. In fact Apple recently broke the internet because some dude thought it was a good idea to leave the braces out.

• Be consistent with your operators. If you're gonna use += to add stuff then you should use *= to multiply stuff.
• Be aware that your constants aren't constant if you store them in an object. Even if you use the const keyword to declare them. That's not how JS "constants" work. You should either rename your constants object to something more accurate (like globals or preferences) or you should store them as individual strings an numbers with the const keyword so they really will be constant and use UPPERCASE variable, as is the convention.
• Semicolons. End your statements with them. If you don't add them then the JS interpreter will guess where to put them and that's a disaster waiting to happen. See this thread for more info.
• Minor nitpick. You should, in my opinion, put a space between your functions to separate them a little bit.
• Please be consistent with your white-space. I don't think you need any blank lines in the first function, but you surely don't need two blank lines before the return.

---

const getMapPointFromCellId = cellId => {
 var {x, y} = getCoordinatesFromCellId(cellId);
 x = x * constants.cellWidth;
 if (y % 2) x += constants.offsetWidth;
 y = y * constants.offsetHeight;
 return {x, y};
};
const getCellIdFromMapPoint = (x, y) => {
 var cellId = 0;
 while (cellId < constants.numberCells) {
 const cellCoordinates = getMapPointFromCellId(cellId)
 const offsetX = cellCoordinates.x + constants.offsetWidth;
 const offsetY = cellCoordinates.y + constants.offsetHeight;
 const endX = cellCoordinates.x + constants.cellWidth;
 const endY = cellCoordinates.y + constants.cellHeight;
 const inRange =
 cellCoordinates.x < x && x < cellCoordinates.x + constants.cellWidth &&
 cellCoordinates.y < y && y < cellCoordinates.y + constants.cellHeight &&
 (
 x < offsetX && y < offsetY &&
 (offsetY - y) / (cellCoordinates.x - x) > (offsetY - cellCoordinates.y) / (cellCoordinates.x - offsetX)
 ) || (
 x > offsetX && y < offsetY &&
 (offsetY - y) / (endX - x) <= (offsetY - cellCoordinates.y) / (endX - offsetX)
 ) || (
 x < offsetX && y > offsetY &&
 (offsetY - y) / (cellCoordinates.x - x) <= (offsetY - endY) / (cellCoordinates.x - offsetX)
 ) || (
 x > offsetX && y > offsetY &&
 (offsetY - y) / (endX - x) >= (offsetY - (constants.cellHeight - constants.offsetHeight)) / (endX - offsetX)
 );
 if (inRange) return cellId;
 cellId++
 }
 return -1;
};

• Gerrit0: I am seriously tempted to downvote for your (presumably supposed to be funny) comic. It is perfectly fine to write JS without semicolons unless your team's style guide tells you to use them. JS Standard style, Blog... I personally prefer no semicolons, but will use whatever the project I am working on currently uses.
• I wrestled a bear once.: yes obviously it was supposed to be funny, but it's also best practice for a number of reasons. if you don't like my answer feel free to downvote but please spare me the petty nonsense next time.
• Gerrit0: Please help me understand the number of reasons then, so far as I can tell using semicolons or not is purely a matter of opinion. The linked post doesn't present compelling proof either for or against. As practically every project includes a linter, and linters can easily warn about the cases that might cause issues.
• I wrestled a bear once.: @Gerrit0 Your advice is to leave out the semicolons "because you can"? Did you read the resources on the linked thread? How about the linked PDF from ECMA international explicitly saying you should always use the semicolons? This question has been asked and answered. If you don't understand it ask for clarification on SO or open a bounty on the linked one.
• Gerrit0: I'm sorry I am not willing to try to understand someone whose first response to a legitimate question is to insult the asker. I did read the linked resources on the top 3 answers. As for the PDF, nope. That's not what it says. I'm done here.