Fixed: lane arrows don't get reset if the lane arrow window is not focused

[kianzarrin]

Fixes #738

Bug:

Lane Arrows do not respond to reset button if the lane arrow tool does not have focus.

Steps to reproduce:

1- change lane arrows for a segment.
2- select another segment and the select the original segment.
3- press delete => lane arrows are not reset.
4- click somewhere inside the window and then press delete => lane arrows are not reset.
5- click a lane arrow button and then press delete => lane arrows (for the segment) are reset.

Why:

ToolWIndow.EventClicked is not triggered until we click a button in the window.

My solution:

Check for key press in every frame

Other Solutions

calling ToolWindow.Focus() after ToolWindow is started in response to EventVisibalityChanged but I think that route is too complicated.

[originalfoo]

Is this related to #738 (comment) ?

[kianzarrin]

@aubergine10 Is this related to #738 (comment) ?

Yes. I should have modified that comment but I slept. why did you re-open that issue though?

[kvakvs]

I prefer it to be done in handlers, instead of checking "IsKeyPressed" every frame. That's just extra work done 60 times per second that eats another 0.001% of the framerate. And we have a LOT of things done every frame.

If this is not working I would reconsider routing key events to the tool via the ModUI root class or something like that, where key handlers work without a problem.

[krzychu124]

I prefer it to be done in handlers, instead of checking "IsKeyPressed" every frame. That's just extra work done 60 times per second that eats another 0.001% of the framerate. And we have a LOT of things done every frame.

@CitiesSkylinesMods/tmpe The problem is that UpdateEveryFrame() not called on every frame but many times per frame
I tested that few days ago and results are a bit surprising.

As we know OnGUI is might be called more than once per frame rendered.

Here is the problem. OnGUI is called at least twice per frame which is:

• once for Layout Event
• then once for Repaint Event
• a few more, for each of input events

Possible scenarios of OnGUI calls within one frame:

Type	Modifier	Button name
Layout	None	button: 0
Repaint	None	button: 0

But when you start hitting keys...

Type	Modifier	Button name
Layout	None	button: 0
Repaint	None	button: 0
Layout	Control, FunctionKey	button: 0
keyDown	Control, FunctionKey	button: 0

Then hold Ctrl+Shift....

Type	Modifier	Button name
Layout	Shift, Control	button: 0
Repaint	Shift, Control	button: 0

Or hold Ctrl+Alt+Shift and Click single time (without modifiers it looks the same) 😄

Type	Modifier	Button name
Layout	Shift, Control, Alt	button: 0
Repaint	Shift, Control, Alt	button: 0
Layout	Shift, Control, Alt	button: 0
MouseDown	Shift, Control, Alt	button: 0
Layout	Shift, Control, Alt	button: 0
MouseUp	Shift, Control, Alt	button: 0

Of course when you now release all keys within one frame it will probably trigger this combination: Layout | Repaint | Layout | keyUp

I am not sure, because it's tricky to test, but i think that if you are getting low fps, chance of multiple OnGUI call is bigger (when game stutters I've got wall of OnGUI events registered between each Update call)

[kvakvs]

New TrafficManagerSubtool (namely LaneArrowTool) does not have OnGUI event i'm very happy about that 👍

[krzychu124]

@kvakvs unfortunately not 😕
I see you use UpdateEveryFrame which is called by (guess what): OnGui().OnToolGUI(Event).OnToolGUIImpl(Event).UpdateEveryFrame()

OnToolGUIImpl(Event) is also called by OnToolGui().Postfix() (when TrafficManager is hidden)

[kvakvs]

In that handler only lane arrows window is repositioned to the node.
I can add a tiny bit of coordinate checking there to do nothing if the camera did not move.

[krzychu124]

You can also use OnToolUpdate, if you need one call per frame.
Revisit of current Tools implementation might be necessary in the future

[kianzarrin]

in CS NetTool uses OnToolGUI to handle IO events.
I can guard my code like this:

psudo code:
if(Event.KeyDown){
    handle event
}

That way it is one if statement per frame.

The current implementation is not much faster. I mean CO.UI handles events by checking OnGUI() all the time. Of course you might argue that CO.UI is doing that anyway so we might as well put it to some good use!

[kianzarrin]

Regarding @krzychu124 comment I noticed this:

namespace UnityEngine
{
	// Token: 0x02000248 RID: 584
	public enum EventType
	{
		MouseDown,
		MouseUp,
		MouseMove,
		MouseDrag,
		KeyDown,
		KeyUp,
		ScrollWheel,
		Repaint,
		Layout,
		DragUpdated,
		DragPerform,
		DragExited = 15,
		Ignore = 11,
		Used,
		ValidateCommand,
		ExecuteCommand,
		ContextClick = 16,
		MouseEnterWindow = 20,
		MouseLeaveWindow,
		mouseDown = 0,
		mouseUp,
		mouseMove,
		mouseDrag,
		keyDown,
		keyUp,
		scrollWheel,
		repaint,
		layout,
		dragUpdated,
		dragPerform,
		ignore,
		used
}

[originalfoo]

I thik we should put some of that info in to a separate issue ticket to revisit it later as likely lots of GUI stuff can benefit from some performacne tweaks?

[kianzarrin]

@kvakvs I added a guard so that now it is only one if statement per call to UpdateEveryFrame() so there is not much performance penalty (not relative to the rest of the code anyways).

If you plan on updating the interface soon to handle events, then we can wait, other lets merge in this fix and then later we can do the performance boost.

[kvakvs]

Its a two sided blade.
I prefer not to amend commits infinitely and get stuff merged and done.
And i'd like to have this done right. But its already working so why not merge it and revisit later.

[originalfoo]

LGTM 👍