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.

Sign up for GitHub

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

Jump to bottom

Provide API abstracting over external/internal levels better #205

Closed
Trouv opened this issue Jul 28, 2023 · 0 comments · Fixed by #244
Closed

Provide API abstracting over external/internal levels better #205

Trouv opened this issue Jul 28, 2023 · 0 comments · Fixed by #244
Assignees
@Trouv
Labels
enhancement New feature or request

Comments

@Trouv
Copy link
Owner

Trouv commented Jul 28, 2023 •
edited
Loading

Currently, LdtkLevel is the de-facto abstraction for retrieving level data in both external and internal cases. A Handle<LdtkLevel> is loaded as a labeled asset in the internal case, and it is loaded as a dependent asset in the external case. However, Since LdtkAsset already has this level data in the internal case, it ends up being cloned in order to create an LdtkLevel. Surely there's a way to leverage a smart pointer and/or enums to avoid this expensive clone.

Something to the effect of...

enum LevelPointer {
    Internal {
        data: Arc<Level>,
        bg_image: Handle<Image>,
    },
    External {
        handle: Handle<LdtkExternalLevel>
    },
}
@Trouv Trouv added the enhancement New feature or request label Jul 28, 2023
@Trouv Trouv self-assigned this Jul 30, 2023
@Trouv Trouv mentioned this issue Aug 8, 2023
Merged
Trouv added a commit that referenced this issue Aug 9, 2023
@Trouv
feat: add LoadedLevel type that wraps around levels with complete data (
3d40c15 
#214)

Works towards #205.

In order to better handle internal/external level storage, it will be
helpful to a have a type that represents "complete" level data. For
projects with internal levels, the normal levels embedded on the
LdtkProject are "complete". For external level projects, the levels on
the main project file are incomplete, and the complete data is stored
separately.

This provides a type for representing complete level data. It does so by
wrapping around `Level` by reference, and enforces the loadedness of the
borrowed level on construction. It also provides immutable getters for
all the level's fields, and `.expects()` away the fields that will not
be null since the level is loaded (`layer_instances`).
@Trouv Trouv mentioned this issue Aug 10, 2023
Merged
Trouv added a commit that referenced this issue Aug 10, 2023
@Trouv
feat: add LevelIid component and spawn it on every level (#215)
ad83455 
Works towards #205.

`Handle<LdtkLevel>` is no longer going to be the main query-able
component on level entities, because "level assets" won't exist in the
internal level case. So instead, `LevelIid` will be the main component
on level entities. There will be a convenient API for retrieving
raw/loaded level data using this `LevelIid` as well. The only part of
this plan that this PR implements is adding the `LevelIid` component and
spawning it on every level.

This is based on the existing `EntityIid` component, but it simplifies
things a little bit. In particular, anything that cloned implicitly
before is gone, and the iid isn't stored as a `Cow` since it won't be
mutable anyway. I'm not against bringing the convenient copying APIs
back if users want them, just thought that that would be a good place to
trim it up a little bit. `EntityIid` might be updated to match in a
future PR.
@Trouv Trouv mentioned this issue Sep 13, 2023
Merged
Trouv added a commit that referenced this issue Sep 14, 2023
@Trouv
feat: add types and traits around LevelMetadata (#229)
382dea2 
Works towards #205.

The asset types are being redesigned a bit to clone less and provide
better APIs. One design goal is to correctly handle both external level
projects and internal level projects. Since we want to avoid loading
level data into a separate asset in the internal level case, these two
scenarios will produce different level metadata on loading. Both need to
map level iids to their indices and background image handles. External
level projects additionally need to produce a handle to the external
level asset. This PR provides `LevelMetadata` and
`ExternalLevelMetadata` types to represent the metadata produced during
project loading.

Mapping iids to level indices with the help of the level metadata types
allows constant-time lookup of levels by iid. This is handy since users
can have access to the level iid thanks to the new `LevelIid` component,
and can use it for finding level data. This also speeds up accessing
levels by `LevelSelection` - two of the 4 level selection variants can
now be constant-time-lookup. To aid in these two use cases, a
`LevelMetadataAccessor` trait is also added in this PR. It can be
implemented for types that store raw level data and a mapping of level
iids to `LevelMetadata`. It provides methods for accessing raw level
data by iid and by `LevelSelection`.

These types/traits have not actually been integrated into `LdtkProject`
in this PR. Doing so is a pretty major change and will be a large PR.
These types/traits have been given their own PR in an attempt to keep
PRs small.
@Trouv Trouv mentioned this issue Oct 16, 2023
Merged
Trouv added a commit that referenced this issue Oct 18, 2023
@Trouv
feat!: redesign LdtkProject with better level data accessors and corr…
670cd4e 
…ect modeling of internal/external levels (#244)

Closes #205

This is the final PR for redesigning the asset types. The main
improvements of this redesign are that the type provides better APIs for
accessing raw or loaded level data, and internal/external levels are
modeled more correctly.

The `LdtkProject` type is still the asset type and stores most metadata
applicable to either level locale, along with `LdtkProjectData`.

`LdtkProjectData` is a enum whose variants store the actual ldtk json
data, with a variant for each level locality. The internal types of the
variants are `LdtkJsonWithMetadata<L: LevelLocale>`, with either
`InternalLevels` or `ExternalLevels` as `L`.

`LdtkJsonWithMetadata<L>` is a generic type storing the actual ldtk json
data + level metadata, with either `InternalLevels` or `ExternalLevels`
as `L`.

Splitting these up like this allows us to define some methods that are
exclusive to each level locality. This is important because accessing
loaded level data is a very different operation memory-wise for each
case (indexing the `LdtkJson` for internal-levels, or accessing the
`Assets<LdtkLevel>` asset store for external levels). But other methods
can be provided for either case, either with a generic implementation at
the lowest level, or exposing transparent implementations in the
higher-level types.

An important point about this new design is that `LdtkLevel` assets are
no longer used in the internal-levels case. Level entities will only
ever have a `LevelIid` component, no longer a `Handle<LdtkLevel>`. The
handle for the asset in the external-levels case is only stored inside
the `LdtkProject`. To help make this change clear, `LdtkLevel` has been
renamed `LdtkExternalLevel`. Doing things this way actually fixes an
egregious clone of all level data that is in all previous versions of
this plugin.

See the documentation of `LdtkProject` to learn about the best ways to
access loaded level data now.

feat!: LdtkLevel renamed to LdtkExternalLevel and is no longer used as a component (#244)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Trouv Trouv

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
1 participant
@Trouv