libtcod / python-tcod

A high-performance Python port of libtcod. Includes the libtcodpy module for backwards compatibility with older projects.
BSD 2-Clause "Simplified" License
404 stars 37 forks source link

Add 'order' argument to tcod.noise.grid #127

Closed TrialOrc closed 1 year ago

TrialOrc commented 1 year ago

Currently, tcod.noise.grid only supports the 'ij' and 'xy' options for indexing. However, it would be useful to have an additional order argument, or to replace the indexing argument with the order argument. The order argument would specify whether the array is in row-major ('C') or column-major ('F') order, which is consistent with how numpy arrays are handled and with the tcod library as a whole.

By adding an order argument, it would make it easier to work with tcod.noise.grid alongside other numpy arrays and tcod. Currently, in order to have a 'F' order array work with tcod.noise.grid, the indexing argument must be set to 'ij', which can feel counter-intuitive.

Overall, adding an order argument would improve the usability and compatibility of tcod.noise.grid.

I've included a quick code snippet:

import tcod
import numpy as np

arr = np.ones((2, 10), order="F")
noise = tcod.noise.Noise(dimensions=2, seed=42)
noise_arr = noise[tcod.noise.grid(shape=(2, 10), scale=0.25, indexing="xy")]
noise_arr_ij = noise[tcod.noise.grid(shape=(2, 10), scale=0.25, indexing="ij")]

print(f"arr (C, F): {arr.flags.c_contiguous, arr.flags.f_contiguous}")
print(f"noise_arr (C, F): {noise_arr.flags.c_contiguous, noise_arr.flags.f_contiguous}")
print(f"noise_arr_ij (C, F): {noise_arr_ij.flags.c_contiguous, noise_arr_ij.flags.f_contiguous}")
arr + noise_arr  # ValueError: operands could not be broadcast together with shapes (2,10) (10,2)
HexDecimal commented 1 year ago

The current behavior is based on numpy.meshgrid which does not provide an order parameter. Since the returned value is for indexing there is no order="F" equivalent parameter other than using indexing="xy" indirectly.

Your example can be fixed with one call to .transpose() or .T:

import tcod
import numpy as np

arr = np.ones((2, 10), order="F")
noise = tcod.noise.Noise(dimensions=2, seed=42)
noise_arr = noise[tcod.noise.grid(shape=(2, 10), scale=0.25, indexing="xy")].transpose()  # Transpose to F-contiguous.

assert arr.flags.f_contiguous
assert noise_arr.flags.f_contiguous
arr + noise_arr  # Works.
HexDecimal commented 1 year ago

Your code snippet was useful. Perhaps I should update the tcod.noise.grid example to show how to transpose the output.

HexDecimal commented 1 year ago

I've updated the tcod.noise.grid example to be more clear. It now shows how to fetch grid samples as an order="F" array.

I'll close this for now since this was more of a documentation issue. I'll answer any other questions you have or resolve a more complex snippet if you have one.