Page MenuHomeWildfire Games

Adds smart search to the Atlas entity list
ClosedPublic

Authored by vladislavbelov on Tue, Jan 7, 2:29 AM.

Details

Reviewers
None
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP23423: Adds non-strict search to the Atlas entity list
Summary

WIth the patch you don't need to enter exact string. For example for units ptol hlr a the first result in the list can be units/ptol_support_healer_a.

The algorithm is pretty simple, can be improved after some feedback.

Test Plan
  1. Apply the patch and compile the game
  2. Check that advanced search works

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

vladislavbelov created this revision.Tue, Jan 7, 2:29 AM
Vulcan added a comment.Tue, Jan 7, 2:31 AM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/968/display/redirect

Vulcan added a comment.Tue, Jan 7, 2:34 AM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/64/display/redirect

Vulcan added a comment.Tue, Jan 7, 2:35 AM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/tools/atlas/AtlasUI/ScenarioEditor/Sections/Object/Object.cpp
| 433| BEGIN_EVENT_TABLE(PlayerComboBox,·wxComboBox)
|    | [MAJOR] CPPCheckBear (unknownMacro):
|    | There is an unknown macro here somewhere. Configuration is required. If BEGIN_EVENT_TABLE is a macro then please configure it.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1486/display/redirect

Vulcan added a comment.Tue, Jan 7, 1:01 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/969/display/redirect

Vulcan added a comment.Tue, Jan 7, 1:03 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/65/display/redirect

Vulcan added a comment.Tue, Jan 7, 1:05 PM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/tools/atlas/AtlasUI/ScenarioEditor/Sections/Object/Object.cpp
| 428| BEGIN_EVENT_TABLE(PlayerComboBox,·wxComboBox)
|    | [MAJOR] CPPCheckBear (unknownMacro):
|    | There is an unknown macro here somewhere. Configuration is required. If BEGIN_EVENT_TABLE is a macro then please configure it.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1487/display/redirect

Nescio added a subscriber: Nescio.Tue, Jan 7, 4:11 PM

Searching in Atlas has been quite frustrating for years so this would be a significant improvement.
However, I haven't been able to reproduce what's described in the summary. What I did was the usual:

svn up
arc patch D2542
cd build/workspaces/
./update-workspaces.sh -j7
cd gcc/
make -j7

and I get this:


It seems Atlas fails to find anything when a space is entered in the search field, as was already the case without this patch. Any ideas what I did wrong?

It seems Atlas fails to find anything when a space is entered in the search field, as was already the case without this patch. Any ideas what I did wrong?

Toggle the checkbox "Advanced search". Maybe it has misleading name.

Nescio added a comment.Tue, Jan 7, 5:36 PM

Toggle the checkbox "Advanced search". Maybe it has misleading name.

Thanks. Toggling the checkbox and then entering the string works. Entering the string first and then clicking the checkbox does not.
Why is there a checkbox anyway? Why not make it the default?

Toggle the checkbox "Advanced search". Maybe it has misleading name.

Thanks. Toggling the checkbox and then entering the string works. Entering the string first and then clicking the checkbox does not.
Why is there a checkbox anyway? Why not make it the default?

Sometimes it might be useful to find only equal names.

Nescio added a comment.Tue, Jan 7, 5:49 PM

Then perhaps make it the default and have an “exact name” checkbox?

Nescio added a comment.EditedTue, Jan 7, 5:58 PM

That inf _b returns all basic infantry templates is simply great, I don't think I would ever want inf _b to return nothing.
However, I find it a bit counter-intuitive hlr returns healer; h l r should.

Then perhaps make it the default and have an “exact name” checkbox?

Yeah, it's possible.

That inf _b returns all basic infantry templates is simply great, I don't think I would ever want inf _b to return nothing.
However, I find it a bit counter-intuitive hlr returns healer; h l r should.

What do you mean? Both hlr and h l r should return the same (for example return healer).

Nescio added a comment.Tue, Jan 7, 6:38 PM

Isn't the space a separator that functions as AND? To me hlr means hlr but h l r means h and l and r. At least that's how search works in programs such as Gnome Nautilus/Files:


vladislavbelov added a comment.EditedTue, Jan 7, 6:50 PM

Isn't the space a separator that functions as AND? To me hlr means hlr but h l r means h and l and r. At least that's how search works in programs such as Gnome Nautilus/Files:

Nope, space is just a character. The search is more near to the sublime`s one, but simpler.

Nescio added a comment.Tue, Jan 7, 6:54 PM

But then I don't understand why you gave units ptol hlr a and not unitsptolhlra or ahillnoprsttu. Besides, there are no entities or actors with a space in their file name.

But then I don't understand why you gave units ptol hlr a and not unitsptolhlra or ahillnoprsttu. Besides, there are no entities or actors with a space in their file name.

For easier reading, because usually it's easier to find a mistype when characters are grouped.

Nescio added a comment.Tue, Jan 7, 7:10 PM

So how does the new search work then?

So how does the new search work then?

You print any characters you want/need. And the algorithm tries to find something most suitable for it. In more technical words it tries to find a word with a longest common subsequence with the search text.

Nescio added a comment.Tue, Jan 7, 7:24 PM

So it will give an ordered list of everything that has at least a single matching character?

So it will give an ordered list of everything that has at least a single matching character?

Currently - yes, but it's ordered by weight. That means most matched items go first.

Nescio added a comment.Tue, Jan 7, 7:34 PM

That's a bit unexpected.
The current function searches for a single string. When I read your summary, I assumed the new function would search for matches for multiple strings, but that's apparently not the case. It would still be an improvement over the exact match, though I doubt someone who enters healer is looking for false positives such as gaia/fauna_pig.

That's a bit unexpected.

That might happen.

though I doubt someone who enters healer is looking for false positives such as gaia/fauna_pig.

It's sublime-like behaviour. Else there might be a lot of different algorithms that may be complex and contradict with each other a bit.

For example what if someone entered healer_, what does it mean, do we need to find a word with _ on the end.

Stan added a subscriber: Stan.Tue, Jan 7, 7:46 PM

Would be nice to have regexp support?

In D2542#106375, @Stan wrote:

Would be nice to have regexp support?

It's possible as a separate checkbox, because it's a much more technical option.

Nescio added a comment.Tue, Jan 7, 8:01 PM

healer_ returns healer_b, healer_a, healer_e times 13 civs in the old function, i.e. 39 units; in the new function the first item is units/kush_support_healer_a, but units/athen_infantry_slinger_b is number seven and skirmish/structures/default_sentry_tower is listed above units/athen_support_healer_a; the item listed last is units/viking_longship.
With sublime I assume you mean some specific software I don't know.
What's intuitive varies from person to person. What I'd prefer is a search function that's somewhere between the old exact single string match and the new single character match.

vladislavbelov added a comment.EditedTue, Jan 7, 8:08 PM

healer_ returns healer_b, healer_a, healer_e times 13 civs in the old function, i.e. 39 units; in the new function the first item is units/kush_support_healer_a, but units/athen_infantry_slinger_b is number seven and skirmish/structures/default_sentry_tower is listed above units/athen_support_healer_a; the item listed last is units/viking_longship.
What's intuitive varies from person to person. What I'd prefer is a search function that's somewhere between the old exact single string match and the new single character match.

So, your suggestion is to put longer matches higher, right?

With sublime I assume you mean some specific software I don't know.

Yeah, text/code editor.

Nescio added a comment.Tue, Jan 7, 8:17 PM

Ideally I'd like a search function that can handle substrings; e.g. ptol healer searches for ptol and healer, thus returning:

units/ptol_support_healer_a
units/ptol_support_healer_b
units/ptol_support_healer_e

The old function returns too little (no matches) for my taste and the new one too much:


(and hundreds of matches more).

The old function returns too little (no matches) for my taste and the new one too much.

I can reorder it and have more useful items first.

A bit unrelated to the patch, but I see the problem with the quarter of screen. Could you provide the system information (system_info.txt might be good)? Also do you have a system scale?

Nescio added a comment.EditedWed, Jan 8, 2:38 PM

A bit unrelated to the patch, but I see the problem with the quarter of screen. Could you provide the system information (system_info.txt might be good)? Also do you have a system scale?


wxWidget version is wxGTK3-devel-3.0.4-10.fc31.x86_64
I've also reported it to you on the forums last year:
https://wildfiregames.com/forum/index.php?/topic/25347-help-with-atlas-map-editor/&tab=comments#comment-368872

Tweaks the algorithm and adds an immediate reaction to the checkbox.

vladislavbelov added a comment.EditedSun, Jan 12, 11:56 PM

@Nescio I've tweaked the algorithm. Now it's a bit closer to your understanding. Now words separated by any non-letter characters should present in a name (it doesn't account underscores, so healer_ a equals to healer a because of possible performance).

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/1021/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/117/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/tools/atlas/AtlasUI/ScenarioEditor/Sections/Object/Object.h
|  23| class·ObjectSidebar·:·public·Sidebar
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classSidebar:' is invalid C code. Use --std or --language to configure the language.

source/tools/atlas/AtlasUI/ScenarioEditor/Sections/Object/Object.cpp
| 506| BEGIN_EVENT_TABLE(PlayerComboBox,·wxComboBox)
|    | [MAJOR] CPPCheckBear (unknownMacro):
|    | There is an unknown macro here somewhere. Configuration is required. If BEGIN_EVENT_TABLE is a macro then please configure it.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1539/display/redirect

Thank you. The new search is much better.

However, my opinion is just one. More people should try it out and comment.

Stan added a comment.Tue, Jan 14, 10:43 PM
source/tools/atlas/AtlasUI/ScenarioEditor/Sections/Object/Object.cpp
361 ↗(On Diff #10995)

negative weight possible? Else maybe size_t here and for function?

363 ↗(On Diff #10995)

!weight?

371 ↗(On Diff #10995)

no way to avoid those conversions? Which by the way look implicit.

elexis added a subscriber: elexis.Tue, Jan 14, 11:22 PM

From blackbox testing, search is acceptable to me, I'd prefer exact as a default too (I dont expect to ever use fuzzy search, but nani implemented it for his mapbrowser and other features too, so perhaps there are users for that).

In D2542#107484, @Stan wrote:

These warnings seem unrelated (they're mostly about many methods of wxListCtrl).

source/tools/atlas/AtlasUI/ScenarioEditor/Sections/Object/Object.cpp
361 ↗(On Diff #10995)

I think I might have an idea to use negative ones. But ok, int isn't necessary here.

363 ↗(On Diff #10995)

Probably.

371 ↗(On Diff #10995)

Nope, it needs to have an explicit cast for different wxWidget versions. Also the function requires const wxString&, so it's not movable.

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/1043/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/139/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/tools/atlas/AtlasUI/ScenarioEditor/Sections/Object/Object.cpp
| 506| BEGIN_EVENT_TABLE(PlayerComboBox,·wxComboBox)
|    | [MAJOR] CPPCheckBear (unknownMacro):
|    | There is an unknown macro here somewhere. Configuration is required. If BEGIN_EVENT_TABLE is a macro then please configure it.

source/tools/atlas/AtlasUI/ScenarioEditor/Sections/Object/Object.h
|  23| class·ObjectSidebar·:·public·Sidebar
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classSidebar:' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1561/display/redirect

This revision was not accepted when it landed; it landed in state Needs Review.Mon, Jan 20, 9:43 PM
This revision was automatically updated to reflect the committed changes.