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

Added filtering by asset type in the AssetBrowser #20025

Merged
merged 3 commits into from
Sep 9, 2022

Conversation

penev92
Copy link
Member

@penev92 penev92 commented May 4, 2022

Closes #3493.

Copy link
Contributor

@dragunoff dragunoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add some padding around the checkboxes:
Screenshot from 2022-05-05 21-08-42

See this for reference:
Screenshot from 2022-05-05 21-09-01

Perhaps we need a better way to create this kind of dropdowns as they are used in several places and as far as remember setting them up them is a bit of a pain.

And on another note - what do we gain from listing unknown types? The asset browser won't be able to display them anyway, right?
image

@penev92
Copy link
Member Author

penev92 commented May 6, 2022

I added spacing:
image
image

Do you want me to do the same for this dropdown in the editor, on which I based mine?
image

I added a filter to not show "non-asset" files (and packages) by excluding unsupported files that are inside the EngineDir. This is what the result looks like:
image
You will notice the audio files in yellow that are not consider "supported" by the asset browser and thus not shown by default.
OK I realized this was a bad example while writing that and fixed the asset browser's supported extensions instead. In any case, with the junk files gone I again feel it is worth to have an actual asset *browser* that can show you the contents of packages.

@dragunoff
Copy link
Contributor

Do you want me to do the same for this dropdown in the editor, on which I based mine? image

Yes, that would be a nice polish up. This one too, please:
image

@penev92
Copy link
Member Author

penev92 commented May 11, 2022

Added spacing in map editor dropdowns, added missing file extension to AssetBrowser in mod manifests and added a filter to not show engine files.

Copy link
Contributor

@dragunoff dragunoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks good but I noticed a couple of things:

  1. Directories are listed as unknown assets but they cannot be clicked on. Shouldn't we filter dirs out of this list?
    Screenshot from 2022-06-02 10-32-17
  2. The scrollpane doesn't update when it's scrolled to the bottom an switching to a list with fewer items:
output.mp4
output.mp4

@penev92
Copy link
Member Author

penev92 commented Jun 3, 2022

The change to fix 1. is squashed into the relevant commit, but here it is standalone:

--- a/OpenRA.Mods.Common/Widgets/Logic/AssetBrowserLogic.cs
+++ b/OpenRA.Mods.Common/Widgets/Logic/AssetBrowserLogic.cs
 	// Don't show unknown files in the engine dir - it is full of code and git and IDE files.
 	// But do show supported types that are inside just in case.
+	// Also don't show "files" without extensions because those may be folders.
 	var fileExtension = Path.GetExtension(file.Key.ToLowerInvariant());
-	if (package.Name == Platform.EngineDir && !allowedExtensions.Contains(fileExtension))
+	if (string.IsNullOrWhiteSpace(fileExtension) || (package.Name == Platform.EngineDir && !allowedExtensions.Contains(fileExtension)))
 		continue;
 
 	AddAsset(assetList, file.Key, package, template);

The change to fix 2. got its own commit.

@abcdefg30
Copy link
Member

Re #20025 (comment): Why do we have to check if there is an extension to determine if it is a folder, and can't use something like IsDirectory or Directory.Exists? (Are we talking about directories inside packages?)

@penev92
Copy link
Member Author

penev92 commented Jun 18, 2022

Are we talking about directories inside packages?

Yes, we're talking about "entries" inside packages, so we can't really check and we resort to guessing.

Also rebased.

dragunoff
dragunoff previously approved these changes Jun 20, 2022
Copy link
Contributor

@dragunoff dragunoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Copy link
Member

@abcdefg30 abcdefg30 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The engine dir problem could also be solved by introducing a blacklist. We currently also display .yaml files in the assets browser which we don't need to...

On a different note, we should think about making the dropdowns in cnc a bit more opaque.
image

Comment on lines +612 to +626
// Don't show unknown files in the engine dir - it is full of code and git and IDE files.
// But do show supported types that are inside just in case.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really like this, as it is hidden and not visible to the outside at all. Besides, when you select unknown, aren't you exactly asking for this? A much better fix would be making the list with packages also one where you can tick/untick packages and maybe have the engine dir unticked by default? (Fwiw, several of the files in the engine dir will already be filtered by the "no extension" rule.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when you select unknown, aren't you exactly asking for this?

Yes, but I was previously asked to hide engine files so I added this 🤷‍♂️

OpenRA.Mods.Common/Widgets/Logic/AssetBrowserLogic.cs Outdated Show resolved Hide resolved
@penev92
Copy link
Member Author

penev92 commented Aug 29, 2022

A much better fix would be making the list with packages also one where you can tick/untick packages and maybe have the engine dir unticked by default?

I agree it would be good, but not in this PR (although that doesn't mean I'm volunteering to do it later either, sorry).

I could add a blacklist, but what do we put in there - extensions? Packages? File names?

Honestly I would like to wrap this (supposedly simple) feature up because the more I stare at the AssetBrowserLogic code the more issues I find that need fixing.

Rebased and fixed the minor nit.

Copy link
Member

@PunkPun PunkPun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise LGTM. This PR is fixing tiny things all over the codebase which are completely unrelated to this PR and be better served in separate PR's. It just unnecessarily makes it harder to merge this

OpenRA.Game/Widgets/Widget.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Widgets/Logic/AssetBrowserLogic.cs Outdated Show resolved Hide resolved
These are all file extensions used by the respective mods ingame, so they should be visible in the asset browser as well.
@penev92
Copy link
Member Author

penev92 commented Sep 2, 2022

Rebased, dropping 4 of the 7 commits as they were merged in #20235.
This PR is now all about filtering the assets.

@PunkPun PunkPun merged commit 99b27bb into OpenRA:bleed Sep 9, 2022
@PunkPun
Copy link
Member

PunkPun commented Sep 9, 2022

Changelog

@penev92 penev92 deleted the assetBrowser branch September 9, 2022 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Asset browser doesn't show anything for mix contents it can't resolve a name for
4 participants