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

Implement dynamic autotiling #338

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Implement dynamic autotiling #338

wants to merge 6 commits into from

Conversation

haath
Copy link

@haath haath commented Nov 7, 2024

Hello!

With this PR I'd like to add support for dynamic auto-tiling.
And by that I mean that layers who are auto-tiled depending on another int grid layer, will now have their tiles automatically updated whenever the values of the corresponding IntGridCell are changed.

You can see this in action in the screencap below, which happens to be captured from the new autotile example that I've also added to this repo.

demo

This is for now a draft PR and a request for feedback.

In particular, I would ideally like some maintainer to help me with the following questions.

  1. Should this functionality always be available, or enabled via a cargo feature?
  2. Should it be always on for all auto layers? Or perhaps controlled by some marker component (e.g EnableAutoLayer)? In the case of the latter, should the marker be added to all newly spawned auto layers by default, or should the user opt-in to this functionality?
  3. The system I've added currently starts by looking up which layers are affected for each int grid layer, and this is done by querying up to find the project and get the information from the json metadata. Since this inter-layer dependency will not change since loading the project though, I propose to cache this information as a new field in LayerMetadata. Do you agree? Or even better in a new component, since this only concerns int grid layers.

The implementation itself is also a work in progress, after we settle on the above I'd like to do some refactoring and maybe also add some tests if it makes sense.

@haath haath changed the title Implement autotiling Implement dynamic autotiling Nov 7, 2024
@haath
Copy link
Author

haath commented Dec 30, 2024

Since the PR went a bit under the radar, I took some liberties with my previous questions to make it ready for the review.

The current design goes like this:

  1. The dynamic autotiling behavior is enabled with a new component called EnableDynamicAutotiling which is added by default to all layers.
  2. A new component on int grid layers called IntGridLayerAffectedLayers keeps the list of the other layer Entity ids which have some dependency on it for autotiling. It gets initialized for newly spawned layers by the new init_int_grid_affected_layers system.
  3. A new component on int grid layers called IntGridLayerCellValues keeps a copy of the 2D array of current int grid cell values on the layer. This is used both to speed up lookups, and to avoid triggering changes unnecessary. This is updated by the new update_int_grid_layer_values system which watches for Changed<IntGridCell>.
  4. The new system apply_int_grid_autotiling watches for Changed<IntGridLayerCellValues> and performs the autotiling on all affected layers accordingly.

I guess some further optimization can be achieved by storing the autotiling rules (which is a Vec<AutoLayerRuleGroup) on each autotiled layer. What do you think?

Also some cases are currently not handled, and they error-out here:

debug_assert!(!rule.perlin_active);
debug_assert!(!rule.flip_x);
debug_assert!(!rule.flip_y);
debug_assert_eq!(rule.checker, Checker::None);
debug_assert!([1, 3, 5, 7, 9].contains(&rule.size));
debug_assert!(!rule.tile_rects_ids.is_empty());
debug_assert!(rule.tile_rects_ids.iter().all(|rect| rect.len() == 1));

@haath haath marked this pull request as ready for review December 30, 2024 18:27
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.

1 participant