aframevr / aframe-inspector

:mag: Visual inspector tool for A-Frame. Hit *<ctrl> + <alt> + i* on any A-Frame scene.
https://aframe.io/aframe-inspector/examples/
MIT License
654 stars 201 forks source link

Texture dialog #376

Closed fernandojsg closed 7 years ago

fernandojsg commented 7 years ago

Modal to manage image assets:

image

image

image

It detects non-valid IDs for the image assets and disable the LOAD TEXTURE button: image

fernandojsg commented 7 years ago

I've updated the PR removing the explicit samples from here and create an assetLoader to fetch the images data from the images registry (https://github.com/fernandojsg/inspector-assets) as proposed on https://github.com/aframevr/aframe-registry/issues/18. I've added a filter functionality to filter by name and tags of the asset: texture_filtering

It needs some work when dealing with modal resize, as it's annoying as it keep adjusting to its contents, so probably when we're on the samples/registry option, just keep that the min/max width/height the same so it won't resize, but you get the point. It also need some optimisations when reading and showing the images as right now they're fully fetched wasting a lot of bandwith. The assets-registry probably should create a compressed thumbnail for each one so we could just show it instead of fetching the whole image even if it's not used. Right now the assets-registry is just a POC of the changes proposed on the aframe-registry if you feel that you like how is going, we could move it to aframevr/assets-registry? and keep fixing it there so we could point this PR to a more stable url than my personal repo.

What do you think @ngokevin @dmarcos @feiss ?

fernandojsg commented 7 years ago

I just noticed how ugly the gif is -_-

feiss commented 7 years ago

Looks nice! 👍

Maybe "Filter..." instead of "Search..."? Yup, a static modal size will be better with the search functionality. Thumbnails will be great, indeed.

the gif looks awesome to me.

fernandojsg commented 7 years ago

updated s/Search/Filter :+1:

dmarcos commented 7 years ago

Should all the dialogs have the same fixed size instead of adapting to the content?

dmarcos commented 7 years ago

Should we allow multiple selection? If you want to add multiple stuff at once instead of going back and forth?

dmarcos commented 7 years ago

Should adding assets to the scene be associated with a-assets? By clicking on the src property of the component you would only see the assets already loaded. If you want to load others you would have to go through the a-assets element. That would match better the underlying model.

fernandojsg commented 7 years ago

Should all the dialogs have the same fixed size instead of adapting to the content?

not really, for this kind of dialogs it's nice to use most of the screen size, but then we have smaller ones like the confirmations, or the shortcuts help that doesn't have enough information to take the whole screen. I'll just add a parameter to the Modal component to define if it's fixed or content-dependent.

Should we allow multiple selection? If you want to add multiple stuff at once instead of going back and forth?

Right now when you launch the texture dialog, you do that by clicking on an texture slot, so you're going to assign just one texture at a time. It's true that it could be nice to keep adding textures from samples to your a-assets even if you're going to add just one that time. But I'll leave this for a v2 of the dialog, as we'll need to arrange it differently the modal, the workflow become a little bit more complex as we need to check if we're adding more than one and allow the user to enter the names of each one while previewing them and so. I'll open an issue for that

Should adding assets to the scene be associated with a-assets? By clicking on the src property of the component you would only see the assets loaded. If you want to load others you would have to go through the a-assets element. That would match better the underlying model.

Right now if you click the src you just see the loaded assets, and if you want to add a new one you follow the current workflow and it will be added into a-assets, creating and id and put that value on src, so we're using all the time references by default to a-assets, instead of url(). Is it what you mean?

dmarcos commented 7 years ago

I see several workflows to load assets. The one I had in mind was when you start a new scene you might have all your assets somewhere but don't know yet what entities will apply to. It would be nice to just click on a-assets in the scene graph and load a folder with your assets library or multiple files at once that you can later use in the entities without having to load each one individually.

dmarcos commented 7 years ago

Another use case of using a-assets is replacing a model in the whole scene. Imagine when developing the a-shooter and you want to replace the models of all the enemies at once to try multiple variations out.

fernandojsg commented 7 years ago

The texture dialog can be launch without be linked to any src parameter, so we could launch it from somewhere else in the UI. What you do when loading the textures is just loading them to the a-assets, so you don't need to apply them to any component. We could use the same dialog for both workflows, the only thing missing in the current one is that you can just drag&drop one file at a time, or select one image from the registry, but allowing it to receive multiple ones should be enough.

fernandojsg commented 7 years ago

Yes, the replace workflow I had also in mind but I didn't make anything yet, it could be nice to be able to launch the dialog, you see the current loaded assets, and you could click on one edit button in one of them and you'll go to the "select texture" part of the modal, and you can upload a new one or select another one from the samples, and when selecting it, you'll change the previous one.

fernandojsg commented 7 years ago

Just opened some issues: https://github.com/aframevr/aframe-inspector/issues/377 https://github.com/aframevr/aframe-inspector/issues/378

dmarcos commented 7 years ago

I cannot evaluate the react part of the code but the feature looks to me. I see a layout problem (in the gif @fernandojsg provided above) in the dialog to load a new texture.

fernandojsg commented 7 years ago

Yes, the gif doesn't look good, I'll try to take a better capture to show the current workflow, and I'll also fix the minWidth/height of that dialog.

fernandojsg commented 7 years ago

I've modified the https://github.com/aframevr/sample-assets generator so it creates a thumbnails with the size of the preview image in the texture modal. So it won't load the whole image just for previewing it. So I believe the only thing left in the PR is to decide what to do with the size of the dialog, if to fit always the content or to set a minimum width/height. 1) Adaptative: textures1

2) force full with/height with some padding textures2

dmarcos commented 7 years ago

I like the panel to take all the space. I would probably put the back button in the title of the panel instead of in the same position than the load texture one

feiss commented 7 years ago

yup, full width or at least, a big % of the screen size. No need for adaptative, because from day 1 there will be many textures available already..

ngokevin commented 7 years ago

+1 on consistent size regardless of content

fernandojsg commented 7 years ago

I've added a hint when you put the mouse over the input as most of the times you can't see the whole URL:

This version includes the asset ID (if it has one) and the fully url image

In the previous version I had just the filename (without the url) just over the canvas: image

Any ideas which one to keep? Maybe added the first version to both?

dmarcos commented 7 years ago

Showing the ID is maybe redundant? I like the complete URL. Maybe if you do double click you could open the image url in a new tab

fernandojsg commented 7 years ago

My idea was to include the hint of the content on any input as sometimes the text could be quite long even if it's just an ID. Regarding the double click is not recommended in react if we want to use in the canvas element that previews the image as it already has a click:

Don't register click and dblclick events on the same element: it's impossible to distinguish single-click events from click events that lead to a dblclick event.

Maybe add that functionality to the icon in the left that shows if it's a asset link (chains), or a fully url (arrow like external link)?

Another option is just to do the extra step to:

fernandojsg commented 7 years ago

Ok, after discussing with @feiss we agreed to remove the previous icon that indicated if the texture was an asset or an url (It doesn't make sense now that we've the input showing the value). And move it to the right and use it as a link for a new tab with the current image: image

fernandojsg commented 7 years ago

Thanks for reviewing it!