-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Add/implement pc.EVENT_MOUSEOUT and pc.EVENT_MOUSEENTER #4920
base: main
Are you sure you want to change the base?
Changes from 5 commits
7c582ab
d8adc32
10d3655
a2c8c28
3faf2ba
6386212
3ba5cc3
2395405
6875316
f7dbb3b
d7233cb
5912854
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -52,6 +52,20 @@ export const EVENT_MOUSEUP = 'mouseup'; | |
*/ | ||
export const EVENT_MOUSEWHEEL = 'mousewheel'; | ||
|
||
/** | ||
* Name of event fired when the mouse moves out or enters another DOM element. | ||
* | ||
* @type {string} | ||
*/ | ||
export const EVENT_MOUSEOUT = 'mouseout'; | ||
|
||
/** | ||
* Name of event fired when the mouse moves out or enters another DOM element. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Slightly confused here since the description is identical to the first sentence of the description for |
||
* | ||
* @type {string} | ||
*/ | ||
export const EVENT_MOUSEENTER = 'mouseenter'; | ||
|
||
/** | ||
* Name of event fired when a new touch occurs. For example, a finger is placed on the device. | ||
* | ||
|
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -50,7 +50,7 @@ class MouseEvent { | |||||||
* @type {number} | ||||||||
*/ | ||||||||
this.y = coords.y; | ||||||||
} else if (isMousePointerLocked()) { | ||||||||
} else if (isMousePointerLocked() || event.type === 'mouseout') { | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why is this necessary? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On the same note, what values are coords.x/y on a mouse out event? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
engine/src/platform/input/mouse-event.js Lines 56 to 58 in 7c582ab
Resulting in the entire event being "uninitialized". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@yaustar There are two cases:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I see 🤔 This seems fine to me |
||||||||
this.x = 0; | ||||||||
this.y = 0; | ||||||||
} else { | ||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,7 @@ | ||
import { platform } from '../../core/platform.js'; | ||
import { EventHandler } from '../../core/event-handler.js'; | ||
|
||
import { EVENT_MOUSEDOWN, EVENT_MOUSEMOVE, EVENT_MOUSEUP, EVENT_MOUSEWHEEL } from './constants.js'; | ||
import { EVENT_MOUSEDOWN, EVENT_MOUSEENTER, EVENT_MOUSEMOVE, EVENT_MOUSEOUT, EVENT_MOUSEUP, EVENT_MOUSEWHEEL } from './constants.js'; | ||
import { isMousePointerLocked, MouseEvent } from './mouse-event.js'; | ||
|
||
/** | ||
|
@@ -16,6 +16,12 @@ import { isMousePointerLocked, MouseEvent } from './mouse-event.js'; | |
* @augments EventHandler | ||
*/ | ||
class Mouse extends EventHandler { | ||
/** | ||
* @type {Element|null} | ||
* @private | ||
*/ | ||
_target; | ||
|
||
/** | ||
* Create a new Mouse instance. | ||
* | ||
|
@@ -36,6 +42,8 @@ class Mouse extends EventHandler { | |
this._downHandler = this._handleDown.bind(this); | ||
this._moveHandler = this._handleMove.bind(this); | ||
this._wheelHandler = this._handleWheel.bind(this); | ||
this._enterHandler = this._handleEnter.bind(this); | ||
this._outHandler = this._handleOut.bind(this); | ||
this._contextMenuHandler = (event) => { | ||
event.preventDefault(); | ||
}; | ||
|
@@ -86,7 +94,7 @@ class Mouse extends EventHandler { | |
/** | ||
* Attach mouse events to an Element. | ||
* | ||
* @param {Element} element - The DOM element to attach the mouse to. | ||
* @param {Element} [element] - The DOM element to attach the mouse to. | ||
*/ | ||
attach(element) { | ||
this._target = element; | ||
|
@@ -99,6 +107,10 @@ class Mouse extends EventHandler { | |
window.addEventListener('mousedown', this._downHandler, opts); | ||
window.addEventListener('mousemove', this._moveHandler, opts); | ||
window.addEventListener('wheel', this._wheelHandler, opts); | ||
if (element) { | ||
element.addEventListener('mouseenter', this._enterHandler, opts); | ||
element.addEventListener('mouseout', this._outHandler, opts); | ||
} | ||
} | ||
|
||
/** | ||
|
@@ -107,13 +119,17 @@ class Mouse extends EventHandler { | |
detach() { | ||
if (!this._attached) return; | ||
this._attached = false; | ||
this._target = null; | ||
|
||
const opts = platform.passiveEvents ? { passive: false } : false; | ||
window.removeEventListener('mouseup', this._upHandler, opts); | ||
window.removeEventListener('mousedown', this._downHandler, opts); | ||
window.removeEventListener('mousemove', this._moveHandler, opts); | ||
window.removeEventListener('wheel', this._wheelHandler, opts); | ||
if (this._target) { | ||
this._target.removeEventListener('mousein', this._enterHandler, opts); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wow, nice catch, absolutely and thank you! |
||
this._target.removeEventListener('mouseout', this._outHandler, opts); | ||
this._target = null; | ||
} | ||
} | ||
|
||
/** | ||
|
@@ -289,6 +305,20 @@ class Mouse extends EventHandler { | |
this.fire(EVENT_MOUSEWHEEL, e); | ||
} | ||
|
||
_handleEnter(event) { | ||
const e = new MouseEvent(this, event); | ||
if (!e.event) return; | ||
|
||
this.fire(EVENT_MOUSEENTER, e); | ||
} | ||
|
||
_handleOut(event) { | ||
const e = new MouseEvent(this, event); | ||
if (!e.event) return; | ||
|
||
this.fire(EVENT_MOUSEOUT, e); | ||
} | ||
|
||
_getTargetCoords(event) { | ||
const rect = this._target.getBoundingClientRect(); | ||
const left = Math.floor(rect.left); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it really "defocused" or "unfocused"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe 'background' is a better term?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated suggested change with something with less jargon.
Edit: I just saw the commit, that looks fine too :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just changed it via normal commit, GitHub is having some strange issues:
(for my sake it sounds good enough, but you can make a suggestion if needed)