Skip to content

fix: tileMove-based functions on a level tilemap#1078

Open
SobennaStory wants to merge 3 commits into
kaplayjs:masterfrom
SobennaStory:tilemove-error
Open

fix: tileMove-based functions on a level tilemap#1078
SobennaStory wants to merge 3 commits into
kaplayjs:masterfrom
SobennaStory:tilemove-error

Conversation

@SobennaStory

Copy link
Copy Markdown

Closes #903

Added options to create spatial map and tilepath-based maps to levelcompopt, as well as more descriptive errors in spatialmap functions. Users should be able to more easily understand and proactively handle tilemove and spatialmap based issus

Comment thread src/ecs/components/level/level.ts Outdated
Comment on lines +114 to +121
/**
* Enable spatial map after adding level tiles
*/
spatialMap?: boolean;
/**
* Enable cost, connectivity, and edge maps after adding level tiles
*/
tilePathMaps?: boolean;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
/**
* Enable spatial map after adding level tiles
*/
spatialMap?: boolean;
/**
* Enable cost, connectivity, and edge maps after adding level tiles
*/
tilePathMaps?: boolean;

Comment thread src/ecs/components/level/level.ts Outdated
Comment on lines +372 to +381

if (opt.spatialMap) {
createSpatialMap(this);
}

if (opt.tilePathMaps) {
createCostMap(this);
createEdgeMap(this);
createConnectivityMap(this);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (opt.spatialMap) {
createSpatialMap(this);
}
if (opt.tilePathMaps) {
createCostMap(this);
createEdgeMap(this);
createConnectivityMap(this);
}

Comment thread src/ecs/components/level/level.ts Outdated
Comment on lines +160 to +167
const ensureSpatialMapExists = () => {
if (!spatialMap) {
throw new Error(
"Spatial map is not initialized. Call level.getSpatialMap() before using spatial map functions, or enable the 'spatialMap' option in level().",
);
}
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
const ensureSpatialMapExists = () => {
if (!spatialMap) {
throw new Error(
"Spatial map is not initialized. Call level.getSpatialMap() before using spatial map functions, or enable the 'spatialMap' option in level().",
);
}
};
const ensureSpatialMapExists = () => {
if (!spatialMap) {
createSpatialMap(this);
}
};

If the spatial map doesn't exist yet ...why not just create it now?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The this inside an arrow function is lexically bound, so it won't refer to the level instance here.
The throw approach sidesteps the problem by making it the caller's responsibility to initialize the map first.

I also mused on

  1. Simply creating the spatial map when the component is attached via add()
  2. Making the tile.tilemove functions throw errors if no spatial map is found in their responsible level
  3. Creating a spatial map whenever level.spawn() is called.

What do you think the best approach is? To implement the above suggestion we'd need to change the props of insertIntoSpatialMap and removeFromSpatialMap which may break other usages of the functions?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

shoot, forgot about that part, that's what I get for not checking this out locally.

to your muses

  1. not necessary for levels that don't use it
  2. please no errors when it could just be created
  3. not necessary for levels that don't use it

I mostly just wanted to reduce the number of edits, so just every place that ensureSpatialMapExists() is called, just replace it with if (!spatialMap) createSpatialMap(this) or if (!spatialMap) createSpatialMap(obj), you might have to make wrappers for insertIntoSpacialMap and removeFromSpatialMap, instead of just using the functions directly, make them wrappers to get the this -

-   insertIntoSpatialMap,
+   insertIntoSpatialMap(obj) { insertIntoSpatialMap(obj, this); }

Comment thread src/ecs/components/level/level.ts
@SobennaStory

Copy link
Copy Markdown
Author

figuring out fmt

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.

bug: calling tileMove-based functions on a level tilemap causes an error

2 participants