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

Updated APIs for objectgroup and get tiled objects via Map #58

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

Conversation

dmtaub
Copy link
Collaborator

@dmtaub dmtaub commented Apr 22, 2021

Considering a couple new APIs for #56

We're cloning objects to include use as entities, so I have been hesitant to just clone the contents of the rs-tiled structs, but maybe it doesn't matter that much. There are two examples here for consideration:

  • passing the Object or Map entity a map.map asset to retrieve the original data (could also work well as an impl-ed trait on the tiled::map) that returns bevy_tiled structures based on indices stored in the Object.
  • shallow clone of tiled structures -- here seen for the case of Object Group, which isn't currently used/cloned anywhere (in my code, at least...)

I'm less worried about cloning object groups, especially shallow-cloned, but I figure there might be orders of magnitude more objects if we improve our spawning API. So, I think both these are mergeable, but @rparrett and others -- lmk what you think about the changes and also about how to proceed:

  1. adding a clone of the tiled::Object and shallow clone of tiled::ObjectGroup as components to each spawned object (probably also the associated Tile, for sprite-like objects)
  2. interface on the Map asset, that, given an object, will borrow the tiled::Object and tiled::ObjectGroup (and optionally tiled::TileSet and tiled::Tile for sprite-like objects)
  3. Continuing with the current design, that constructs custom structures that clone in tiled::Foo data, to be used as components

@rparrett
Copy link
Contributor

I am not sure if I understand the use cases for users that want bevy_tiled to automatically spawn their objects well enough to provide useful commentary.

I am trying to imagine scenarios where that is desirable, and this may just me a lack of imagination, but not coming up with much.

In my case, a workflow like

  • Load map
  • Map spawns objects into ECS
  • Query objects via ECS (not really filterable at this point beyond With<bevy_tiled_prototype::Object> or With<tiled::Object>)
  • Iterate over every object and adding/removing components based on object.obj_type, object.props, object_group

Seems a bit roundabout. I'm currently doing something like

  • Load map
  • Access map via Res<Assets<Map>>
  • Iterate over every object and spawn entities/components based on object.obj_type, object.props, object_group

But for half of my objects or so, I don't think I would want auto-spawning to happen at all, and then I'd have two totally different methods of setting up objects.

I think that my ideal API might be something like being able to provide a closure that gets called to spawn objects for a particular layer, or a particular object_type. But my Rust isn't good enough to know if this sort of thing is remotely doable.

Would love to hear from anyone else using Objects.

@rparrett
Copy link
Contributor

I do have some objects though that ideally would have graphics from the tileset associated with them, and it's sort of a pain to deal with if bevy_tiled isn't spawning them for me.

Is it possible to, outside of bevy_tiled, spawn a SpriteSheetBundle with some particular tile from the tileset?

@dmtaub
Copy link
Collaborator Author

dmtaub commented Apr 23, 2021

One of the missing bits from your example is that I listen for object creation and then attach markers depending on the object's properties. I find myself not knowing the "user wants to spawn objects themselves" use case well enough, so we're even :) I did figure it should be possible to call object.spawn().add(MarkerComponent) one or more times for any object in the map, and I'd like to build that API relatively soon.

Another consideration is - do we want to, on load, create texture atlases for all the tiles in the map? I noticed that there is missing a "Tile" API though there remain Layer, ObjectGroup, and Map analogues in bevy_tiled, so we could store texture handles in a similar field for Tile and have a Map.get_atlas(Object) for the spawning you mention. Perhaps the tiles and objects could be spawned as entities by default but without any graphical objects attached, and the library user could provide an "attach_sprite()" method or user could instead query for the object (now with a texture handle) and spawn it manually if they need?

My goal will be sensible defaults that someone can just drop in a Tiled map and it will do something usable. For special use cases, we can build out the API -- this might mean that the "sensible defaults" is a plugin that you include if you want the map to automagically spawn visible objects -- though I still wonder if we should always spawn object entities even if they aren't renderable....

@rparrett
Copy link
Contributor

Ah, I see. I was completely ignorant of ObjectReadyEvent. (even using MapReadyEvent is on my todo list). That seems pretty ideal, really. I'm not as familiar with the internals and new features as I could be, so forgive my uninformed opinions.

I have some objects that end up getting set up as Resource rather than a set of Entities though, so those ending up in the ECS just feels a bit odd. If I were building from scratch with knowledge of ObjectReadyEvent, I might reconsider that design.

All three options sound pretty workable, really.

Option 1

If broken up like (MinimalObjectToMakeBevyTiledWork, tiled::Object, tiled::ObjectGroup), you could perhaps just remove the components you do not need, or will no longer have a use for after they have spawned. I like that, but adding and removing a component feels like wasted work. But if someone were concerned about that, maybe some sort of configuration could be added so that those don't get included? Still, seems "bevy-ish."

Option 2

This seems somewhat clunky but efficient.

Option 3

This is pretty convenient, but maybe somewhat inelegant?

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.

2 participants