How does the user know that he has to doubleclick? Isn't atlas supposed to activate the placer tool again once the objects tab is activated again (i.e. independent of the means to activate the panel)?

Isn't atlas supposed to activate the placer tool again once the objects tab is activated again (i.e. independent of the means to activate the panel)?

Not sure I understand, but the placement tool doesn't get activated by selecting the object tab, it becomes active by selecting an object from the list.

How does the user know that he has to doubleclick?

As stated, the placement tool activates by selecting, i.e. clicking, an object in the list, so they will naturally expect to reactivate it by clicking. BUT unfortunately they will expect to reactivate it by a single click, not doubleclick - they will probably figure out doubleclick works, but the ideal solution would be to make it work with a single click.

The reason I bound it on doubleclick is that it's a one liner fix for an issue that doesn't occur very often. Making it singleclick would be more difficult, because wxListBox emits events on either item change or doubleclicking an item, but not singleclicking an already selected item. There might be a way around this but as I'm new to wxwidgets I can't see any simple one - all I can think of is subclassing the wxListBox and change its behavior - should I try it? Or maybe anyone sees a simpler solution? Or will this one be sufficient?

Can't we do something like a fake click ?

In D1588#63738, @Stan wrote:

Can't we do something like a fake click ?

It's not good. It's a workaround.

The problem, that we don't have a visible tool for the object placement. So I can suggest to add this tool. It can be together with the doubleclick.

Yeah so I guess that's not gonna be a one liner.
One could also add a simple click event. And call that a feature.

I'm not good with wxWidgets either, doesn't EVT_COMMAND_LEFT_CLICK do what we want? http://docs.wxwidgets.org/3.1/classwx_command_event.html (I don't recall if we still support wx 3.0, not unlikely do)
(Notice that we'd still need the EVT_LISTBOX event for keyboard induced selectionchanges as far as I see)

In D1588#63744, @elexis wrote:

I'm not good with wxWidgets either, doesn't EVT_COMMAND_LEFT_CLICK do what we want? http://docs.wxwidgets.org/3.1/classwx_command_event.html (I don't recall if we still support wx 3.0, not unlikely do)
(Notice that we'd still need the EVT_LISTBOX event for keyboard induced selectionchanges as far as I see)

Tried EVT_COMMAND_LEFT_CLICK - compiles but doesn't work. The reference says it's "wxMSW only", so that may be why.

I'll be looking into other options.

In D1588#63740, @vladislavbelov wrote:

The problem, that we don't have a visible tool for the object placement. So I can suggest to add this tool. It can be together with the doubleclick.

Adding a placement tool clickable icon might be a solution - could it be like this?

• Add a Place object tool button to the toolbar (alongside the other main tools).
• The Place tool would allow placing the object that's currently selected in the objects list (even if the object list tab wouldn't be active, e.g. you could have the "Map" tab active and still be able to place objects with the tool).
• If no object is currently selected in the list (after program start), simply grey the icon out? Or make it just do nothing? Don't know here.

In D1588#63746, @drummyfish wrote:

Adding a placement tool clickable icon might be a solution - could it be like this?

• Add a Place object tool button to the toolbar (alongside the other main tools).
• The Place tool would allow placing the object that's currently selected in the objects list (even if the object list tab wouldn't be active, e.g. you could have the "Map" tab active and still be able to place objects with the tool).
• If no object is currently selected in the list (after program start), simply grey the icon out? Or make it just do nothing? Don't know here.

It can be the same as the the paint tool for terrain (to be consistent). If a texture isn't selected, then a terrain would be painted with a default selected texture.

But I can't imagine that we need new UI elements to solve this problem.
As far as I see we only need to call OnSelectObject if the user activated the ObjectSidebar tab, no?

In D1588#63748, @elexis wrote:

But I can't imagine that we need new UI elements to solve this problem.
As far as I see we only need to call OnSelectObject if the user activated the ObjectSidebar tab, no?

True, it solves the original problem in a "programmer" way. But for a user it can be unusual to click on the already selected item again to enable the placement tool. Also without visual confirmation you can't understand is the placement tool enabled or you're just moving an invisible object. So the visual tool is need for more usability IMO.

Another (more near to @elexis thoughts) way is to deselect the item, when the placement tool is disabled.

Wait, the problem is that if you enter the object tab, select an item, then select a different tab, then select the object tab again, the placement is disabled, but the user expectation is that the placement of the already selected object is enabled.
So calling OnSelectObject when the object tab is selected fixes that without the user noticing anything, without adding any new GUI elements, right?

In D1588#63750, @elexis wrote:

Wait, the problem is that if you enter the object tab, select an item, then select a different tab, then select the object tab again, the placement is disabled, but the user expectation is that the placement of the already selected object is enabled.
So calling OnSelectObject when the object tab is selected fixes that without the user noticing anything, without adding any new GUI elements, right?

Nope, there are at least 2 other similar problems:

1. Switch tool (no tab change)
2. Switch Actor View

So you suggest for any new place duplicate the call, right?

New GUI elements aren't necessary. They just increases the usability a little bit IMO.

In D1588#63751, @vladislavbelov wrote:

Nope, there are at least 2 other similar problems:

1. Switch tool (no tab change)
2. Switch Actor View

Ah, you are right, I wasn't aware of these two cases.

So you suggest for any new place duplicate the call, right?

No, solving issues without duplication is preferable if there is a feasible way to omit it and if there are more than 1-3 statements repeated (and if it doesn't change the logic of several unrelated pieces of code just before a release).

deselect the item, when the placement tool is disabled.

That might work if there is a single event to subscribe to.
(If we have to subscribe to multiple events that result in disabling then we might consider subscribing to multiple events that result in enabling of the placement tool) and chose the way of least code complexity and maximum user satisfaction.)

I think the real issue here is the lack of a proper "place object" cursor mode. If you select an object to place then you get an object placement cursor, but if you switch to anything else then there's no way to get back the object you were placing except to select some other object first to get the object placement cursor back, then switch to the object you actually wanted. Most of the other cursors have proper modes so if you switch to a mode you get whatever it was you were last using with it, but object placement doesn't have any indicator that that's what you're even doing until you start moving the cursor around and see the object on it.

Yeah, if I changed the current double click solution to a single click, that would be an acceptable minimal solution to the ticket. But adding the new Placement tool/mode is better IMHO as it solves a deeper underlying problem. Adding unnecessary UI is definitely bad, but here it makes sense, is consistent with the rest of the GUI and makes it more predictable, because the rule seems to be:

Exactly one tool (mode) of the toolbar is active at any time.

That's what the user will expect. But then there is the placement tool/mode which is a weird and confusing exception to this, plus it creates the problem described in the ticket.

Sooo I am going to give it a try here.

Try it out please. A new icon for the tool would have to be made, for now I gave it the same one as the Move tool. ATM if you start the editor and select the Place tool without selecting any object first, a warning is written out in the window - should I leave it like this, or try to grey the button out? Or maybe automatically select the first item in the list? Not sure.

In D1588#63771, @drummyfish wrote:

Try it out please. A new icon for the tool would have to be made, for now I gave it the same one as the Move tool. ATM if you start the editor and select the Place tool without selecting any object first, a warning is written out in the window - should I leave it like this, or try to grey the button out? Or maybe automatically select the first item in the list? Not sure.

I think default is good, because as I mentioned above, we have the default logic for the Paint Terrain tool.

Also it seems we maybe don't need the g_LastObjectID to be global, because the restoring works for the Paint Terrain tool without globals (but it stores some information outside).

I think default is good, because as I mentioned above, we have the default logic for the Paint Terrain tool.

I agree, but what should the default be? The Paint tool has "grass1_spring" hard-coded - should I hard-code a value the same way? It's not good, someone can use a mod where the file won't exists, but this issue can probably be left for a later dehardcoding stage, right?

Also it seems we maybe don't need the g_LastObjectID to be global, because the restoring works for the Paint Terrain tool without globals (but it stores some information outside).

/source/tools/atlas/AtlasUI/ScenarioEditor/ScenarioEditor.cpp:401: g_SelectedTexture = _T("grass1_spring");

^ it is using a global for the default, would be good to copy this behavior, right? But it's declared in MiscState.cpp, so maybe I should move g_LastObjectID in there too?

Could I ask for feedback again?

If this was the way to go, I could try to make a new icon for the Placement tool. Don't know how to submit it though when it's not diffable - how are these things handled?

In D1588#63775, @drummyfish wrote:

I agree, but what should the default be? The Paint tool has "grass1_spring" hard-coded - should I hard-code a value the same way? It's not good, someone can use a mod where the file won't exists, but this issue can probably be left for a later dehardcoding stage, right?

It'd be good to get first item from the list.

/source/tools/atlas/AtlasUI/ScenarioEditor/ScenarioEditor.cpp:401: g_SelectedTexture = _T("grass1_spring");

^ it is using a global for the default, would be good to copy this behavior, right? But it's declared in MiscState.cpp, so maybe I should move g_LastObjectID in there too?

I think it'd be good to have all globals in one place for easier refactoring.

In D1588#63831, @drummyfish wrote:

Could I ask for feedback again?

Sure, you could ping reviewers here or on IRC or PM on the forum.

If this was the way to go, I could try to make a new icon for the Placement tool. Don't know how to submit it though when it's not diffable - how are these things handled?

You could use arcainst: https://trac.wildfiregames.com/wiki/Phabricator#UsingArcanist. Or you can just upload it here as an attachment.

In D1588#63834, @vladislavbelov wrote:

It'd be good to get first item from the list.

I agree, but:

1. The object list loading is deferred to only when the panel is first displayed. I'd have to load it as soon as the program starts - good idea to drop this "feature" or not?
2. ATM I hard-coded it the same way the default texture is hardcoded - ideally the texture should be also loaded as the first one in the texture list. Both of these should be fixed at the same time - should I therefore fix the texture along, or leave it for later as another issue fix?
3. What if the list is empty?

I think it'd be good to have all globals in one place for easier refactoring.

Moved it.