June 2022 - Wine-Devel - list.winehq.org

Re: [PATCH 0/1] MR181: winebus: Optionally split joysticks with more than 6 axes.

by Rémi Bernon

Rémi Bernon (@rbernon) commented about dlls/winebus.sys/bus_sdl.c: > SDL_GameController *controller = NULL; > const char *str; > char guid_str[33]; > + int i; > + int axis_count = 0; NP: Please use grouped declarations. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/181#note_1651

June 7, 2022

Re: [PATCH 0/1] MR181: winebus: Optionally split joysticks with more than 6 axes.

by Rémi Bernon

Rémi Bernon (@rbernon) commented about dlls/winebus.sys/bus_sdl.c: > + int offset_name_length = snprintf(NULL, 0, " %d", i+1); > + /* If there is not enough place to append " %d" replace some of the last characters. */ > + int offset_name_start = min(strlen(str), ARRAY_SIZE(desc.product) - 1 - offset_name_length); > + /* Ensure we only write in desc.product and not past it. > + * Otherwise just keep the name as is. */ > + if (offset_name_length < 12 && offset_name_start >= 0 && offset_name_start + offset_name_length < ARRAY_SIZE(desc.product)) > + { > + snprintf(offset_name, 12, " %d", i+1); > + ntdll_umbstowcs(offset_name, 12, desc.product + offset_name_start, 12); > + } > + TRACE("%s id %d, split for axis %d-%d.\n", controller ? "controller" : "joystick", id, 6*i, min(axis_count, 6*(i+1)-1)); > + } > + > + bus_event_queue_device_created(&event_queue, &impl->unix_device, &desc); > + ++i; > + } while (options.split_controllers && !desc.is_gamepad && 6*i < axis_count); This could be simpler, without needing a special i != 0 case. You can fill the product buffer with something like that in the loop, before the `hid_device_create` and original `TRACE` call: ```C++ char buffer[ARRAY_SIZE(desc.product)]; if (axis_offset) snprintf(buffer, ARRAY_SIZE(buffer), "%s %u", product, axis_offset / 6); else snprintf(buffer, ARRAY_SIZE(buffer), "%s", product); ntdll_umbstowcs(buffer, strlen(buffer) + 1, desc.product, ARRAY_SIZE(desc.product)); ``` With `product` the string retrieved from pSDL_JoystickName, replaced with a default value if it was `NULL`. Imho you could use a for loop there, and replace i with an axis_offset variable. The increment would depend on `options.split_controllers`. The `desc.is_gamepad` case above should set `axis_count = 6`, so you don't have to bother about afterwards. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/181#note_1650

June 7, 2022

Re: [PATCH 0/1] MR181: winebus: Optionally split joysticks with more than 6 axes.

by Rémi Bernon

Rémi Bernon (@rbernon) commented about dlls/winebus.sys/bus_sdl.c: > { > id = ((SDL_JoyButtonEvent *)event)->which; > impl = find_device_from_id(id); > + if (impl && options.split_controllers && event->type == SDL_JOYAXISMOTION) { > + axis_offset = ((SDL_JoyAxisEvent *)event)->axis / 6; > + if (axis_offset > 0) > + { > + ((SDL_JoyAxisEvent*)event)->axis = ((SDL_JoyAxisEvent*)event)->axis % 6; > + impl = find_device_from_id_and_offset(id, axis_offset); > + } > + } I think this would be cleaner in a dedicated earlier `else if (event->type == SDL_JOYAXISMOTION && options.split_controllers)` block. Please also keep the original indentation and bracing styles. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/181#note_1649

June 7, 2022

Re: [PATCH 0/1] MR181: winebus: Optionally split joysticks with more than 6 axes.

by Rémi Bernon

Rémi Bernon (@rbernon) commented about dlls/winebus.sys/bus_sdl.c: > struct sdl_device *impl; > > LIST_FOR_EACH_ENTRY(impl, &device_list, struct sdl_device, unix_device.entry) > - if (impl->id == id) return impl; > + if (impl->id == id && impl->axis_offset == 0) return impl; > + > + return NULL; > +} > + > +static struct sdl_device *find_device_from_id_and_offset(SDL_JoystickID id, unsigned int offset) > +{ > + struct sdl_device *impl; > + > + LIST_FOR_EACH_ENTRY(impl, &device_list, struct sdl_device, unix_device.entry) > + if (impl->id == id && impl->axis_offset == offset) return impl; With the `axis_offset` change described below I think this should take an axis number as parameter, and look for a device which matches `impl->axis_offset <= axis && impl->axis_offset + impl->unix_device.hid_device_state.abs_axis_count > axis`. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/181#note_1648

June 7, 2022

Re: [PATCH 0/1] MR181: winebus: Optionally split joysticks with more than 6 axes.

by Rémi Bernon

Rémi Bernon (@rbernon) commented about dlls/winebus.sys/bus_sdl.c: > USAGE_AND_PAGE physical_usage; > > axis_count = pSDL_JoystickNumAxes(impl->sdl_joystick); > + if (options.split_controllers && axis_count > 6) > + { > + if (impl->axis_offset == axis_count / 6) > + { > + axis_count = axis_count % 6; > + } > + else > + { > + axis_count = 6; > + } > + } Overall I'd say that the `axis_offset` field is a bit misleading, as it's not an offset but an index of 6-axis groups. I think it'd be better to have it truly an offset, so that devices may be split differently than in groups of 6 axes in the future without requiring too many changes. For now it's okay to keep a hardcoded 6 axes limit here and in the device creation loop below. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/181#note_1647

June 7, 2022

Re: [PATCH 0/1] MR181: winebus: Optionally split joysticks with more than 6 axes.

by Rémi Bernon

Rémi Bernon (@rbernon) commented about dlls/winebus.sys/main.c: > }; > NTSTATUS status; > > + bus_options.split_controllers = check_bus_option(L"Split Controllers", 0); > + if (bus_options.split_controllers) TRACE("SDL controller splitting enabled\n"); > + NP: Please remove this empty line. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/181#note_1646

June 7, 2022

Re: [PATCH 0/1] MR181: winebus: Optionally split joysticks with more than 6 axes.

by Rémi Bernon

Rémi Bernon (@rbernon) commented about dlls/winebus.sys/unixlib.h: > UINT input; > UINT uid; > BOOL is_gamepad; > + UINT axis_offset; This isn't used anywhere. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/181#note_1645

June 7, 2022

Re: [PATCH 0/1] MR181: winebus: Optionally split joysticks with more than 6 axes.

by Rémi Bernon

Rémi Bernon (@rbernon) commented about dlls/winebus.sys/bus_sdl.c: > USAGE usages[16]; > > if (!pSDL_JoystickIsHaptic(impl->sdl_joystick) || > - !(impl->sdl_haptic = pSDL_HapticOpenFromJoystick(impl->sdl_joystick))) > + !(impl->sdl_haptic = pSDL_HapticOpenFromJoystick(impl->sdl_joystick)) || > + impl->axis_offset > 0) I thknk it would be better with the `!impl->axis_offset ||` condition first. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/181#note_1643

June 7, 2022

Re: [PATCH 0/1] MR181: winebus: Optionally split joysticks with more than 6 axes.

by Rémi Bernon

Rémi Bernon (@rbernon) commented about dlls/winebus.sys/bus_sdl.c: > impl->effect_support |= WINE_SDL_HAPTIC_RUMBLE; > } > > - if (pSDL_JoystickRumble && !pSDL_JoystickRumble(impl->sdl_joystick, 0, 0, 0)) > + if (pSDL_JoystickRumble && !pSDL_JoystickRumble(impl->sdl_joystick, 0, 0, 0) && impl->axis_offset == 0) Same here. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/181#note_1644

June 7, 2022

Re: [PATCH vkd3d] vkd3d-shader/spirv: Do not print a warning for VKD3D_SHADER_COMPILE_OPTION_API_VERSION.

by Henri Verbeet

Signed-off-by: Henri Verbeet <hverbeet(a)codeweavers.com>

June 7, 2022