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

WiimoteEmu: Major cleanups #7674

Open
wants to merge 22 commits into
base: master
from

Conversation

8 participants
@jordan-woyak
Copy link
Contributor

jordan-woyak commented Jan 4, 2019

This is a step towards making emulated MotionPlus fully functional, making wiimote TAS/Netplay work better, and possibly supporting real wiimote input in the Dolphin UI / ControllerInterface.

I've reorganized the emulated wiimote code into multiple files and classes. (It was one giant mega class in a mega file)
I've tried to make all the giant functions into small manageable ones.
The I2C bus which a real wiimote uses to communicate with the camera, speaker and extensions is now simulated.
Simulating this bus is needed for a clean MotionPlus emulation as it does some trickery with changing addresses.

I've improved the accuracy of the emulated wiimote's behavior based on testing I've done on physical wiimotes. Most of this was concerning edge cases and error codes so the changes are going to be mostly unnoticed, but I've added lots of comments and added names to lots of magic numbers.

I've improved how data reports are created and modified.
The old ReportFeatures struct been replaced by DataReportBuilder (WiimoteCommon/DataReport.h) which abstracts away the data offsets and bit-hacks needed to handle the various formats of button and accelerometer data.
The TAS code is somewhat cleaner with this.

The "Interleaved" reports and "Full IR" camera data are now implemented but untested because I am not aware of a game that actually uses them. (Fishie Fishie and Cricket Challenge only enable Full IR for half a second on boot)

I've tried to make the names of things more consistent so the old wiimote Attachment class has been renamed Extension and the ControlGroup Extension is renamed Attachments (with an s) because it handles the configuration of choosing one of many attachments (not necessarily a wiimote extension).

I've reorganized what should and and shouldn't be wiimote state in the Wiimote class and updated the save-state version.

FYI: The emulated MotionPlus is now enabled and should simulate a "stable" sensor. 25-ish% of the time it will fail a certificate check and you will get a PanicAlert. More investigation is needed to fix this. Nunchuck pass-through is also not working. Non-motion plus games are unaffected because they do not attempt to activate the motion plus of course.
Edit: M+ is now disabled in code until it's mature.

Minor things:

  • Renamed types/variables to comply with our code style.
  • Improved type-safety (enum classes, removing memsets, etc).

jordan-woyak added some commits Nov 22, 2018

WiimoteEmu: Partially emulate i2c bus to more closely simulate the re…
…al thing. Transfer most of IR camera logic to the i2c bus. Temporarily break everything else.
WiimoteEmu: Process wiimote read data requests like they are on a rea…
…l wiimote. It's not a queue. New requests are ignored and input is suppressed while processing a request. This simplifies the save state code greatly.
WiimoteEmu: Cleanup ack handling. Improve accuracy of report handling…
… with unusual values. Eliminated outdated comments.

@jordan-woyak jordan-woyak force-pushed the jordan-woyak:wiimote-emu-cleanup branch 3 times, most recently from 208aae2 to 8a021d7 Jan 4, 2019

Show resolved Hide resolved Source/Core/Core/State.cpp Outdated

@jordan-woyak jordan-woyak force-pushed the jordan-woyak:wiimote-emu-cleanup branch from 8a021d7 to 743fa26 Jan 4, 2019

@JMC47

This comment has been minimized.

Copy link
Contributor

JMC47 commented Jan 4, 2019

Testing wise, this appears to not cause any new issues. There are thousands of games, so I particularly targeted games that were problematic. Motion+ games are noted to be broken by Billiard, so I didn't do much with them aside from squee when they went in-game for those precious moments before the game realized something was wrong.

1: Boom Blox - Variable Shakes still working
2: New Super Mario Bros. Wii - Verified 4 player still works. Nunchuck mode works. It also refuses to play when a guitar is plugged in (which is accurate to console)
3: Bully - Still boots with emulated Wii Remotes (this game is very touchy)
4: Far Cry: Vengeance - Still says I'm sitting too far away
5: Deadly Creatures - Shake works fine
6: Rune Factory: Tides of Destiny - Shake Still Works.
7: Jett Rocket is still a terrible game.

I tested a few other games and features as well.

New Super Mario Bros. Wii - TAS Input + Input recording. This feature works as intended it seems.
Savestates - seem to work across several titles.
Wii Remote IR is much more responsive in a game like Mario Party 8 where you need to precisely paint a dinosaur onto a shed.
Also tested swapping into/out of incorrect attachments to try and mess up things but it works the same as master. A game will detect the wrong attachment and make you switch. I did get one invalid write, but, that happens on master as well and is out of the scope of the cleanup.

@JosJuice

This comment has been minimized.

Copy link
Contributor

JosJuice commented Jan 4, 2019

Just to confirm, are games with optional MotionPlus support also broken by this?

@jordan-woyak

This comment has been minimized.

Copy link
Contributor

jordan-woyak commented Jan 4, 2019

@JosJuice I wasn't familiar with optional M+ games. I'll disable the M+ emu until it's functional.

@Helios747

This comment has been minimized.

Copy link
Contributor

Helios747 commented Jan 5, 2019

How do you intend to expose WM+ controls to the user, and deal with the problem that every game uses the data differently? "Down Swing" in one WM+ game doesn't equal "Down Swing" in another.

@jordan-woyak

This comment has been minimized.

Copy link
Contributor

jordan-woyak commented Jan 5, 2019

@Helios747 My plan is to allow mapping an analog-stick/mouse to what would be the movement of the wiimote in an outstretched arm. This should at least make Skyward Sword and Wii Sports Resort playable.

@Helios747

This comment has been minimized.

Copy link
Contributor

Helios747 commented Jan 5, 2019

A solution that works in two games isn't really selling me on this.

@Helios747

This comment has been minimized.

Copy link
Contributor

Helios747 commented Jan 5, 2019

Additionally, that may work for swings, but what about tilting, orientation?

@iwubcode

This comment has been minimized.

Copy link
Contributor

iwubcode commented Jan 5, 2019

I skimmed the code a bit and loving the cleanup and comments. I'm not sure how much I'll be able to contribute but I do plan on giving it a more in-depth look soon.

@jordan-woyak - is this code complete or do you plan on waiting until the M+ is more accurate?

I don't believe we need to expose M+ to users in this PR. Also not sure if it's wise to advertise it until there's a clear plan on how to present it to users. However, just from the code cleanup and the potential for more accurate emulation, I think this is great!

@Helios747

This comment has been minimized.

Copy link
Contributor

Helios747 commented Jan 5, 2019

Agreed. I'm not opposed to having the framework down for initial WM+ support, especially since real hardware is getting harder to find, but the UX for this is going to be a nightmare to figure out no matter how you slice it. It would have been nice to have OpenXR so we could just say "get a controller with a gyro" but my hair will probably start going grey before that ever becomes a thing.

Is there a list of WM+ enabled games so we have an idea on what the implementation needs to do? Or for the sake of sane UX, I'd be ok with a reasonable implementation that covers the majority of games.

@lioncash
Copy link
Member

lioncash left a comment

In the future, consider not dumping a 4K+ LoC slab of changes as one PR, and instead breaking it out into smaller PRs. Several of these would have been fine on their own, and also been way less of a pain to review individually.

Show resolved Hide resolved Source/Core/Common/BitUtils.h
void SetBit(T& value, size_t bit_number, bool bit_value)
{
if (bit_value)
value |= (1 << bit_number);

This comment has been minimized.

@lioncash

lioncash Jan 5, 2019

Member
Suggested change Beta
value |= (1 << bit_number);
value |= (T{1} << bit_number);

This prevents undefined behavior in cases like 1 << N - 1 where N is the size of a type in bits (so 1 << 31 with a 32-bit unsigned, type etc).

This comment has been minimized.

@jordan-woyak

jordan-woyak Jan 5, 2019

Contributor

👍

if (bit_value)
value |= (1 << bit_number);
else
value &= ~(1 << bit_number);

This comment has been minimized.

@lioncash

lioncash Jan 5, 2019

Member
Suggested change Beta
value &= ~(1 << bit_number);
value &= ~(T{1} << bit_number);

This comment has been minimized.

@jordan-woyak

jordan-woyak Jan 5, 2019

Contributor

👍


void GetCoreData(CoreData* result) const override
{
*result = *reinterpret_cast<const CoreData*>(data_ptr);

This comment has been minimized.

@lioncash

lioncash Jan 5, 2019

Member

Please don't. Use memcpy instead, since it doesn't actually invoke undefined behavior. Ditto below for all usages as a type-punning mechanism below.

class DataReportBuilder
{
public:
DataReportBuilder(InputReportID rpt_id);

This comment has been minimized.

@lioncash

lioncash Jan 5, 2019

Member
Suggested change Beta
DataReportBuilder(InputReportID rpt_id);
explicit DataReportBuilder(InputReportID rpt_id);

This comment has been minimized.

@jordan-woyak

jordan-woyak Jan 5, 2019

Contributor

👍

Show resolved Hide resolved Source/Core/Core/HW/WiimoteEmu/MotionPlus.cpp
if (0x18 == reg_data.cert_ready)
{
// TODO: determine the meaning of this
const u8 mp_cert2[64] = {

This comment has been minimized.

@lioncash

lioncash Jan 5, 2019

Member
Suggested change Beta
const u8 mp_cert2[64] = {
const std::array<u8, 64> mp_cert2 = {

This comment has been minimized.

@jordan-woyak

jordan-woyak Jan 5, 2019

Contributor

👍

constexpr u8 EXT_ADDR = ExtensionPort::REPORT_I2C_ADDR;

// Try to alternate between M+ and EXT data:
auto& mplus_data = *reinterpret_cast<DataFormat*>(data);

This comment has been minimized.

@lioncash

lioncash Jan 5, 2019

Member

Please don't. This is undefined behavior. Memcpy what you need into the data.

This comment has been minimized.

@jordan-woyak

jordan-woyak Jan 5, 2019

Contributor

I know it's UB. This appears a lot in Dolphin. Could/should we add some kind of BitCastArray function to BitUtils for situations like this? I don't think we should just have memcpys strewn about.

@@ -23,6 +23,37 @@ class PointerWrap;

namespace WiimoteReal
{
using WiimoteCommon::MAX_PAYLOAD;

typedef std::vector<u8> Report;

This comment has been minimized.

@lioncash

lioncash Jan 5, 2019

Member
Suggested change Beta
typedef std::vector<u8> Report;
using Report = std::vector<u8>;

This comment has been minimized.

@jordan-woyak

jordan-woyak Jan 5, 2019

Contributor

👍

Show resolved Hide resolved Source/Core/Core/Movie.h Outdated
@jordan-woyak

This comment has been minimized.

Copy link
Contributor

jordan-woyak commented Jan 5, 2019

@ Helios747 I guess we can worry about how users will control the M+ once the hardware is figured out. :)

@jordan-woyak

This comment has been minimized.

Copy link
Contributor

jordan-woyak commented Jan 5, 2019

@iwubcode I'd say this is complete enough for now. Motion plus can be enabled later once it's figured out.

@jordan-woyak

This comment has been minimized.

Copy link
Contributor

jordan-woyak commented Jan 5, 2019

@lioncash Regarding the reinterpret_cast usage being UB. How do you feel about adding something like this to BitUtils? https://ideone.com/091UPt (I realize it is lacking a few static_assert)

@@ -235,7 +235,7 @@ Wiimote::Wiimote(const unsigned int index) : m_index(index)
m_options->numeric_settings.emplace_back(
std::make_unique<ControllerEmu::NumericSetting>(_trans("Speaker Pan"), 0, -127, 127));
m_options->numeric_settings.emplace_back(
m_battery_setting = new ControllerEmu::NumericSetting(_trans("Battery"), 95.0 / 100, 0, 255));
m_battery_setting = new ControllerEmu::NumericSetting(_trans("Battery"), 95.0 / 100, 0, 100));

This comment has been minimized.

@riking

riking Jan 6, 2019

Contributor

200, not 100?

This comment has been minimized.

@jordan-woyak

jordan-woyak Jan 6, 2019

Contributor

Yes. I don't think we need to expose this to the user though. Let them configure the level from 0 to 100.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment