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

Implement mouse_move event #434

Closed
wants to merge 6 commits into from
Closed

Conversation

Lemmmy
Copy link
Contributor

@Lemmmy Lemmmy commented Apr 30, 2020

This implements the first half of #433. I'm not actually interested in implementing term.getMousePosition() from that issue out of fear of issues with multiple players, and having to implement some kind of polling system for that. Such a feature may be better suited for the window API.

When the cursor is moved within the terminal on any client, a mouse_move event is issued with the xPos and yPos parameters (in characters, as usual). If the cursor moves from inside to outside the terminal's region, the position -1, -1 is sent. This allows the program to deactivate any element that is currently hovered. This program demonstrates that by turning the screen red (with throttle disabled):

mouse_move

To solve the issue with bandwidth, there is a config option for throttling, mouse_move_throttle. By default, this is 200ms, and is implemented as a leading-edge debounce. 200ms will be fine for servers, as I wouldn't recommend anyone build programs with a 'cursor' anyway (due to issues raised in #409). Instead, this is more useful for things like buttons. The feature can be disabled entirely by setting the throttle time in the config to -1.

It's worth noting that mouse_drag does not have a throttle, but those events are less likely to happen compared to mouse_move events. It's simple to copy the code to implement a drag throttle if needed, though.

Multiple players in a computer works no differently to mouse_drag, that is - the events are simply multiplexed with no distinction between players. I think this will be fine, given that the events are only sent if the mouse actually moves.

One more concern is throttle configuration. I've simply used the same configuration as everything else at the moment. In my understanding of Forge, mod configs are synchronised to the player when they connect, but I don't know if there's anything that stops the player from reconfiguring it client-side. It may be worth reviewing this.

I've tested this on all kinds of computers (Computer, Pocket Computer, Turtle, Neural Interface). Basic computers do not transmit mouse events at all, so this works perfectly.

TO-DO

  • Fix multishell/window API support
  • Update CCEmuX
  • Port to 1.14+

Thoughts

  • I'm not 100% sure about sending -1, -1 as the position for the mouse being outside the terminal. Perhaps nil, nil would be better, but it could break programs that perform blanket handling of mouse events? The other solution is another event, e.g. lose_focus.
  • The multishell code is ugly, as a result of not sending the additional argument with the mouse event. Perhaps it should be sent, but always as nil or 0, just to be fully compatible with all the other mouse events?

Thanks

@plt-amy
Copy link
Contributor

plt-amy commented Apr 30, 2020

If anyone really wants a getMousePos it's easy enough to Do It Yourself™, in true CC fashion:

local last_x, last_y
function term.getMousePos()
  return last_x, last_y
end
parallel.waitForAny(function()
  -- your code
end, function()
  while true do
    local _, x, y = os.pullEvent('mouse_move')
    last_x, last_y = x, y
  end
end)

@znepb
Copy link
Contributor

znepb commented Apr 30, 2020

Nice! Works like a charm. Thanks for implementing this. I made a test program with the build Lemmmy provided here. Here's the link on pastebin: https://pastebin.com/lelkewh8

@Lemmmy
Copy link
Contributor Author

Lemmmy commented Apr 30, 2020

If anyone wants to test it out, grab the latest artifact here.

@SquidDev SquidDev added area-Core This affects CC's core (the Lua runtime, APIs, computer internals). area-Minecraft This affects CC's Minecraft-specific content. enhancement An extension of a feature or a new feature. feedback wanted Tell me what you want, what you really really want. labels Apr 30, 2020
@SquidDev SquidDev linked an issue Apr 30, 2020 that may be closed by this pull request
Copy link
Member

@SquidDev SquidDev left a comment

Choose a reason for hiding this comment

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

My instinct is to say "no", because I'm a grumpy grump who hates change, but let's see how this goes for now.

- Mouse throttling has been moved to the server, specifically to the
  InputState class.
- The default mouse_move throttle time is now 50ms.
- When the cursor is moved outside of the terminal, or the container
  is closed, `nil` is now sent instead of `-1`.
- Added an extra 'dummy' argument to ensure compatibility with programs
  performing blanket handling of mouse events.
@Lemmmy
Copy link
Contributor Author

Lemmmy commented May 1, 2020

That lint failure definitely wasn't me. I've made the requested changes:

  • Mouse throttling has been moved to the server, specifically to the InputState class.
  • The default mouse_move throttle time is now 50ms.
  • When the cursor is moved outside of the terminal, or the container is closed, nil is now sent instead of -1.
  • Added an extra 'dummy' argument to ensure compatibility with programs performing blanket handling of mouse events.

With these changes, the effect is now mouse_move 1 x y, making it compatible with all the other mouse events. The x and y coordinates, however, are now nil, which is not compatible with the other mouse events entirely. For example, I had to add a special case to multishell's handler for the y position.

@Lemmmy Lemmmy requested a review from SquidDev May 1, 2020 07:36
@SquidDev
Copy link
Member

SquidDev commented May 1, 2020

Ahh, that lint failure is my fault. Looks like an illuaminate update caught some more stuff :). I really need to do something which yells at me whenever changes break master.

@osmarks
Copy link
Contributor

osmarks commented May 2, 2020

I've been asked to provide feedback on this. Personally I'm not really a fan - it seems as if it would spam computers with events and thus worsen performance, have problems with multiple simultaneous users (I think the suggestion was to send events from the last player who clicked, but that would be really unintuitive), and it doesn't seem to provide much value. Hover-based UIs could be very annoying to use. Also, it wouldn't translate neatly to monitors, so some programs would be unable to work on those.

@Luca0208
Copy link
Contributor

Luca0208 commented May 4, 2020

I'd agree on osmarks with the event spam, especially because most of the time the mouse_move events will just be ignored. I would suggest adding a function which enables/disables the mouse_move events, kind of like you need to use modem.open to receive modem_message events.

I'd put the function either in term or in os. If it is put in term, monitors could implement the method by just throwing an error informing the user, that mouse_move events don't work with monitors. Of course that would mean that there would be programs which work on computers, but crash when run on monitors, although this would only affect new programs, so I'm not sure how big of a problem that would be.

@Lemmmy
Copy link
Contributor Author

Lemmmy commented May 4, 2020

I totally agree with the event spam. As a server admin, this was my biggest concern, so I kept this in mind the entire time while implementing. What do you think about a toggle @SquidDev ?

For clarification, the events are throttled server-side and on default configuration won't be sent more than once per 50ms (this was originally 100-200 before Squid requested it to be lowered). On SwitchCraft, this will not be any less than 200ms, possibly higher. That means that no more than 5 mouse_move events will be sent per second. For comparison, mouse_drag events are sent more than once per tick.

The biggest concern with the event spam is people who have a loop like this:

while true do
  local event = os.pullEvent()
  -- ...
  redraw()  
end

This will result in significantly more unnecessary redraws. However, I am not actually concerned about this as an admin. This is why we have profilers (/computercraft track) and analytics, and as @osmarks and @Luca0208 know, we are no stranger to telling people to fix their bad program design. If anything, this will make it easier to weed out bad programs.

Additionally, on SwitchCraft, most of the terminal drawing overhead actually comes from monitors that are updating at regular intervals 24/7. It's not really that common that a player is moving their mouse around in a terminal. If they're in the shell or edit, both hands are on the keyboard and the mouse is not moving. The mouse only ever moves if they are using a GUI, and it doesn't move as frequently as you think. And actually, in my testing, key events are more of an event spam problem (key, key_up, char) than mouse_move.

@SquidDev
Copy link
Member

SquidDev commented May 4, 2020

Thanks to the joys of the aforementioned analytics, we can actually crunch the numbers!

Some statistics about events

This is a plot of the number of events computers process each second on SC (in the last hour).

  • Computers on average process 1.4 events per second.
  • 95% of computers process 12 or less.
  • Some computers may process up to 100-150/s. Fun facts, as I'm writing this there's one computer handling 1k events in a second.

Given that most computers are not being interacted with, and events are being debounced/limited to 20/s (or 5/s on SC), I don't think the additional event spam is really something to be worried about.1

I think it's worth referring to #408, where we had a similar discussion about event spam. I think it's worth bearing in mind that CC is designed around an event system - if it can't handle a large number of them, it's doing something wrong.

Obviously this is something we'll monitor on SC, and this can be throttled/disabled if it becomes a problem.


As far as monitors go, while true I don't think it's a major issue. There's no counterpart for mouse_up or mouse_scroll events after all.


1: As Lem says, a decent typing speed sends a lot of more events. There's 3 for every character!

- The throttling timer is now tick-based, and thus monotonic
- Un-refactored the WidgetTerminal code
- Fixed event documentation
@dmarcuse
Copy link
Contributor

dmarcuse commented May 5, 2020

As far as out-of-bounds events go, I think it'd make more sense to non-numeric values (i.e. nil) or even better, a separate event. This is a bit more consistent with existing input events (e.g. mouse_up and key_up are separate from mouse_click and key) and would make the API easier to understand and use IMO.

@SquidDev
Copy link
Member

SquidDev commented May 5, 2020

Maybe mouse_leave? Possibly mouse_exit, but I think that's less clear.

It occurs to me we should probably fire that in multishell, when the cursor enters the menu bar at the top too.

@znepb
Copy link
Contributor

znepb commented May 5, 2020

As with mouse_leave, I think if would be best if term_focuslost was implemented instead. This would solve the problem with programs checking for "mouse_" in the event string, as having mouse_exit would cause problems (fuzzy event checking, as said before).

@SquidDev
Copy link
Member

SquidDev commented May 5, 2020

I'm incredibly unfussed about people doing event:find("^mouse_"). I don't think it's reasonable to worry about every possible broken thing people may be doing.

@znepb
Copy link
Contributor

znepb commented May 5, 2020

I'm incredibly unfussed about people doing event:find("^mouse_"). I don't think it's reasonable to worry about every possible broken thing people may be doing.

I used this in zwm, and when the mouse_move update was pushed to SC, it broke it. So I'm rewriting all the event forwarding 😛

As with term_focuslost, this would break older programs that are no longer maintained. I'll try to find some examples of this that use fuzzy mouse searching (or whatever) and give some examples here. As for now, returning nil, nil works.

@SquidDev
Copy link
Member

SquidDev commented May 5, 2020

The reality is that we can't ensure forwards compatibility for bad code. Obviously I don't want to break anything, but I also don't want to make things wildly inconsistent with the rest of CC for the sake of dealing with every eventuality.

MCJack123 added a commit to MCJack123/craftos2 that referenced this pull request May 10, 2020
This is because cc-tweaked/CC-Tweaked#434 is still in progress. I don't
want to ship CraftOS-PC with features that aren't complete by default to
avoid causing some compatibility problems once the feature is complete.
The mouse_move event will still be available as it was in previous
builds, but it will require setting `mouse_move_throttle` to a positive
value to be enabled.
@SquidDev
Copy link
Member

SquidDev commented Jul 3, 2020

See my comments in #433. Sorry @Lemmmy!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Core This affects CC's core (the Lua runtime, APIs, computer internals). area-Minecraft This affects CC's Minecraft-specific content. enhancement An extension of a feature or a new feature. feedback wanted Tell me what you want, what you really really want.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Functions/events to make more responsive UIs
8 participants