-
-
Notifications
You must be signed in to change notification settings - Fork 21.1k
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
Minor code cleanup in xr_nodes
#70240
base: master
Are you sure you want to change the base?
Minor code cleanup in xr_nodes
#70240
Conversation
* Reference parameters * Add const where possible * Remove semicolon at the end of some method implementation * `vp_he` -> `viewport_half_extents` * Simplify some if else logic * Remove some useless break * Remove already explicit comments "get our XRServer" Signed-off-by: Quentin Guidée <quentin.guidee@gmail.com>
8061dfd
to
48f40f2
Compare
} | ||
|
||
void XRNode3D::trigger_haptic_pulse(const String &p_action_name, double p_frequency, double p_amplitude, double p_duration_sec, double p_delay_sec) { | ||
void XRNode3D::trigger_haptic_pulse(const String &p_action_name, double p_frequency, double p_amplitude, double p_duration_sec, double p_delay_sec) const { |
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.
Not entirely sure if it makes sense to make this const. Haptics do change internal state even if this is offloaded to the XR interface, unless it's trigger_haptic_pulse
is also const (which it shouldn't be)
I'm not against these changes, some are nice cleanups others are just semantics. Especially passing some of the parameters as references is probably a good idea. |
Needs a rebase as well as addressing the review comments. |
Clean xr_nodes
vp_he
->viewport_half_extents