search

Trouv / bevy_ecs_ldtk

ECS-friendly ldtk plugin for bevy, leveraging bevy_ecs_tilemap
Other
641 stars 73 forks source link

add reflect too LdtkLevel and all parts #201

Closed Hellzbellz123 closed 1 year ago

Hellzbellz123 commented 1 year ago

closed earlier commit and reopened with my main branch and commits derives Reflect and FromReflect on the necessary parts for LdtkLevel and i added too World too but its not used mostly just used too visually debug things in egui_inspector, but can be useful elswhere.

i have reflect_ignore on the real_editor_values Vec because its external

i need a git crash course, i tried too rebase from main but it would then ask me too pull down changes from my fork, i just rebased my branches main kinda on accident but its fixed now

BEGIN_COMMIT_OVERRIDE feat: register and derive Reflect for LdtkLevel and dependent types (#201) END_COMMIT_OVERRIDE

Trouv commented 1 year ago

i need a git crash course, i tried too rebase from main but it would then ask me too pull down changes from my fork, i just rebased my branches main kinda on accident but its fixed now

I see! No worries - git is a complicated thing, and what you've done here accomplishes what I was asking for!

Trouv commented 1 year ago

BTW - it sounds like your forks main branch pulled this forks 0.8 branch is that right? So it makes sense that git rebase -i main wouldn't really work since you're developing on your fork's main branch, and even if you weren't developing there, your main branch still had those changes and such a rebase wouldn't give you the option to exclude them. I have a couple tips for this..

First, you can undo a pull on your main branch. A branch is essentially just a name pointing to some commit in the repository, and you can change what commit a branch is pointing to via a "reset". This is kind of a dangerous operation, but you could run something like git reset HEAD^ --hard to point the branch you're using to the previous commit (HEAD^ is a short name for the previous commit), essentially deleting the most recent commit. Obviously you can lose work this way, so be careful using it.

Also, to help me organize my forks, I like to have a couple "remotes" set up. One remote "origin" pointing to my fork (which you pretty much get by default when you clone your fork), and then one remote "upstream" pointing to the main repository. You can add the "upstream" remote pretty easily using whichever link you'd use to clone this repository:

git remote add upstream git@github.com:Trouv/bevy_ecs_ldtk.git

And from there you can interact with upstream branches, rather than just your local ones or your fork's branches. So, you could probably have accomplished that rebase command I suggested without any resetting by changing it to git rebase -i upstream/main.

Hope this is good for future reference, sorry if some of this is stuff you already know.

Trouv commented 1 year ago

So, the field_instances example actually provides a bevy_inspector_egui window, and I'm not seeing the asset in there still. I think maybe you need to register some of these types to the app as well via App::register_type.

I'm not completely sure how it works, maybe you can just register LdtkAsset and LdtkLevel and that's all you need? It's possible some of the dependent types would need to be registered too, but I'd like to minimize that if possible.

Hellzbellz123 commented 1 year ago

My workstation died, currently setting up a new environment

Pretty sure everything on ldtklevel and level will need to be registered. I don't think .register_type() works recursively yet it might be useful too reflect ignore more types depending on the data and its usefulness but I'm pretty sure we got everything that should be ignored already

Trouv commented 1 year ago

Turns out the method we need is App::register_asset_reflect, and we only need to use it on the top-level.

Interestingly, when I go to expand the LdtkAsset in that example, apparently the level despawns as I'm running into this .expect() with an error NoEntities. It seems that expanding triggers an AssetModified event, and we handle these events by respawning the world, which probably isn't the correct behavior.

I think this needs to be addressed somehow before merging. We could change the AssetModified event handling, but that sounds like it will be difficult and could easily introduce bugs. Instead, I'll take back my previous suggestion of implementing Reflect on LdtkAsset, and ask to leave it on LdtkLevel only, since expanding LdtkLevel doesn't have this problem. We can take more drastic measures in future PRs if inspecting the LdtkAsset is something people want to do.