The class NonCopyable is, like the name says, supposed to disallow
copying. But should it allow moving?
For a long time, NonCopyable used to not allow moving. (It declared
a deleted copy constructor and assigment operator without declaring
a move constructor and assignment operator, making the compiler
implicitly delete the move constructor and assignment operator.)
That's fine if the classes that inherit from NonCopyable don't need
to be movable or if writing the move constructor and assignment
operator by hand is fine, but that's not the case for all classes,
as I discovered when I was working on the DirectoryBlob PR.
Because of that, I decided to make NonCopyable movable in c7602cc,
allowing me to use NonCopyable in DirectoryBlob.h. That was however
an unfortunate decision, because some of the classes that inherit
from NonCopyable have incorrect behavior when moved by default-
generated move constructors and assignment operators, and do not
explicitly delete the move constructors and assignment operators,
relying on NonCopyable being non-movable.
So what can we do about this? There are four solutions that I can
think of:
1. Make NonCopyable non-movable and tell DirectoryBlob to suck it.
2. Keep allowing moving NonCopyable, and expect that classes that
don't support moving will delete the move constructor and
assignment operator manually. Not only is this inconsistent
(having classes disallow copying one way and disallow moving
another way), but deleting the move constructor and assignment
operator manually is too easy to forget compared to how tricky
the resulting problems are.
3. Have one "MovableNonCopyable" and one "NonMovableNonCopyable".
It works, but it feels rather silly...
4. Don't have a NonCopyable class at all. Considering that deleting
the copy constructor and assignment operator only takes two lines
of code, I don't see much of a reason to keep NonCopyable. I
suppose that there was more of a point in having NonCopyable back
in the pre-C++11 days, when it wasn't possible to use "= delete".
I decided to go with the fourth one (like the commit title says).
The implementation of the commit is fairly straight-forward, though
I would like to point out that I skipped adding "= delete" lines
for classes whose only reason for being uncopyable is that they
contain uncopyable classes like File::IOFile and std::unique_ptr,
because the compiler makes such classes uncopyable automatically.
I'm not sure why this hasn't popped up as an error on the buildbots,
but the build fails on my new install of VS2017. The error is C2445:
result type of conditional expression is ambiguous: types 'wxString'
and 'const char [1]' can be converted to multiple common types
to get bigger, breaking an optimization. This forces the emitter to use a
32bit pointer instead of an 8bit one, fixing the issue at the expense of
efficiency.
Seems like I was wrong that ANDI2R doesn't require a temporary register here.
There is *one* case when the mask won't fit in the ARM AND instruction:
mask = 0xFFFFFFFF
But let's just use MOV instead of AND here for this case...
Currently, GameFile returns a generic banner if the file didn't have one
available (either because the file format doesn't support it, or because
it's a Wii file without an associated save).
It makes more sense to handle the lack of banner in the UI layer. The
game list will use the generic missing banner explicitly (no change from before), and the game info window now omits the banner display entirely if the file didn't have one (since it's not useful to display/allow the user to save the "missing banner" banner).
Given a relatively recent proposal (P0657R0), which calls for deprecation of putting stuff into the global namespace when using C++ headers, this just futureproofs our code a little more.
Technically this is what we should have been doing initially, since an
implementation is allowed to not provide these types in the global
namespace and still be compliant.
It's strange to see GameTracker add its own initial paths in
construction, because you might expect a race condition where the
GameLoaded signal is emitted before it gets connected to in
GameListModel.
In fact, this doesn't happen, but only because of how it abuses the Qt
signals mechanism to load files asynchronously: GameLoader emits a
GameLoaded signal which gets forwarded to the GameTracker::GameLoaded
signal _after_ control returns to the event loop, at which point
GameListModel has connected.
This commit moves the logic of adding initial paths out of GameTracker
to a point after the signals are connected, which is more obvious and
doesn't rely on how GameTracker implements concurrency.
ifstream::read() sets the failbit if trying to read over the end, which
means that (!input) would be hit for the 'last' block if it wasn't
exactly BSIZE (1024) bytes.
Makes it easier to turn off general IOS messages that can be
distracting (e.g. /dev/net/ssl being opened hundreds of time...)
without losing the ability to view WFS messages.
The opagent library was (incorrectly) marked as a dependency for "Core"
instead of "Common".
When linked with --as-needed, any symbols the linker can tell are not
used are discarded. As the link is done in command-line order, and the
Core library (and dependencies) are processed before Common, it would
link in Core, then opagent, but as at that point no opagent symbols are
used the whole opagent library would be discarded.
Moving the opagent library to be a dependency of Common fixes this, as
after the Common library is linked, there *are* opagent symbols used.
This helpers are not for general CR calculation, they are just for the
common case of the sign extended result of integer instructions if the
rc bit is set.
They must not be used by other instructions like cmp, so there is no
need to be as flexible.
cmpi shall compare two signed 32 bit values. The used difference a-b
may overflow and so the resulting 32 bit value can't represent it.
A correct way would be cr = s64(a) - s64(b) and it should be done in
this way in the JITs, but the Interpreter shall implement the most
readable way.
Also drops the now unused helper function.
The switch statements in these functions appear to get transformed into
an if..else chain on NVIDIA's OpenGL/Vulkan drivers, resulting in lower
performance than the D3D counterparts. Transforming the switch into a
binary tree of ifs can increase performance by up to 20%.
Since we don't want users to have to configure the region manually
and always enforce one automatically, we should fall back to a region
that was likely to be chosen by the user instead of always using
PAL whenever the title region cannot be detected.
Dolphin doesn't mess with installed NAND titles like the system menu,
so it is a reliable indicator of what region the user wants.
"Pad size" just doesn't make much sense. Let's go with "Buffer size"
instead, since the control for it is labeled "Buffer".
(Another possibility is "Pad buffer size", but I'm against that,
because we've stopped referring to controllers as "pads" in almost
all GUI strings.)
* Add missing Language setting loading/saving. This was added after the
original OnionConfig PR, which is why support for it was missing.
* Change MovieConfigLoader to reuse ConfigInfos. Less duplication.
* Extract MovieConfigLoader::Save into SaveToDTM. The DTM should use
the current config and not just the movie layer. This makes more
sense than just saving the movie layer, which may not always exist,
and also fixes a crash that would happen when creating a new
recording because the movie layer wouldn't exist in that case.
(Plus, having to get the loader from the layer and call ChangeDTM
on it manually is not very pretty.)
Not really used anywhere yet, but useful for not having to duplicate
config locations and for getting rid of conflicts when I get around
to rebase my Main.Core and Main.DSP porting PR.
Settings that come from the SYSCONF are now included in Dolphin's
config system as part of the base layer. They are handled in a
special way compared to other settings to make sure they are only
loaded from and saved to the SYSCONF (to avoid different, possibly
contradicting sources of truth).
This could cause the first branch of the bootucode procedure, which
takes its parameters from the AX registers, to run during the ROM init
sequence. Since the ROM doesn't set any of the AX registers, the values
aren't meaningful, and can cause bad DMA transfers and crashes.
This adds a WiiWad constructor that takes a BlobReader, so that the
class can be used with more than just files from the host filesystem.
Required for using WiiWad with WADs from update partitions.
Must be 9 characters at most; otherwise the serial number will be
rejected by SDK libraries, as there is a check to ensure the string
length is strictly lower than 10.
The casts to u32* are technically undefined behavior. The u8* cast is
left, as char/unsigned char is exempted from this rule to allow for
bvtewise inspection of objects (and this is what s8/u8 are typedefs of
on platforms we support).
Writing to 0x60 does actually not "init exception[s]" or anything like
that. Not at all. Rather, it *breaks* a check in Nintendo's SDK, which
makes it fail to realise that the hook hasn't been set up.
This prevents the SDK initialisation routines from writing the rest of
the hook instructions (total: 0x20 bytes), which in turn causes an
anti-piracy check to fail in some Ubisoft games (including Tintin).
Dolphin can be really amazing sometimes.
No clue where people got the 0 value from, or why it's labelled as
"time". As far as I can tell, it is always set to 0xffffffff by
official NAND titles, including the system menu.
It's not specific to WADs. The BS2 emulation boot code will also need
to update the state file.
Move the struct to Boot and add a helper function that will handle
reading + computing the checksum + writing the state file.
Sentret_C posted this comment on Transifex recently:
"What Dolphin refers to as "Table View" and "List View" are
similar to "List View" and "Grid View" in Steam, and I think
the Steam names describe them better."
I agree with that, so here's a commit that changes the names.
This is supposed to get efb2tex to the same texture as efb2ram, by applying the related efb copies as updates after each other, in the order of their creation.
Improve bookkeeping around formats. Hopefully make code less confusing.
- Rename TlutFormat -> TLUTFormat to follow conventions.
- Use enum classes to prevent using a Texture format where an EFB Copy format
is expected or vice-versa.
- Use common EFBCopyFormat names regardless of depth and YUV configurations.
This way it allows us to use surfaceless contexts in EGL/GLX. It also
ensures that the shared context shares a similar setup to the main
context's framebuffer, potentially reducing the number of variants a
driver needs to generate.
Previously we were falling back to an earlier version of the compiler.
The older version cannot compile our ubershaders without various
graphical issues.
When recording during netplay, the stop message was only sent after you
have chosen a filename for the replay, causing the other player(s)
to freeze for a few seconds. This takes care of the annoyance.
There were two problems with this:
1. If the starting offset was beyond the end of the disc,
we would dereference an invalid iterator.
2. The data beyond the end of the disc was non-deterministic.
Having DiscContents with size 0 would mean that some DiscContents
might not get added to the std::set because of them comparing
identically to another DiscContent.
This replaces an older piece of code in WriteDirectory that ensures
that no two files have the same starting offset. (We now care about
the ending offset, not the starting offset. The new solution both
ensures that no two files have the same ending offset and that no
two files have the same starting offset.)
For instance, we don't want to show TGC files that might be
inside the /files/ directory of a GameCube DirectoryBlob,
and we don't want to show the /sys/main.dol files for extra
partitions of Wii DirectoryBlobs.
Now it's clearer that SetDOL depends on SetApploader
and BuildFST depends on SetDOL.
As a side note, we now load the DOL even if there's
no apploader. (I don't think it matters whether we
do it, but it was easier to implement this way.)
This lets VolumeDirectory/DirectoryBlob skip implementing
various volume functions like GetGameID, GetBanner, etc.
It also lets us view extracted discs in the game list.
This ends up breaking the boot process for Wii
DirectoryBlobs due to workarounds being removed from the
boot process, but that will be fixed later by adding
proper DirectoryBlob support for things like TMDs.
We now expect the directories to be laid out in a certain
format (based on the format that WIT uses) instead of requiring
the user to set the DVD root and apploader path settings.
This is useful for blob types that store Wii data unencrypted
(such as WIA and discs extracted to directories) so that
we don't have to waste CPU time encrypting in the blob code
just to decrypt right afterwards in the volume code.
The old approach to detecting DOL/ELF files doesn't fit
with the new way of implementing extracted discs.
The game list is already doing it in a way that's similar
to the approach that this commit uses.
The Config::AddLoadLayer functions call Load on the layer
explicitly, but Load is already called in the constructor,
so they'd cause the loader's Load function to be called twice,
which is potentially expensive considering we have to read an INI
from the host filesystem.
This commit removes the Config::AddLoadLayer functions because
they don't appear to be necessary.
This was mainly included for debugging, but could end up being confusing
for users, as well as polluting the GL program cache with a mix of uber
and specialized shaders if the option was changed.
On Windows, File::GetTempFilenameForAtomicWrite returns a path
somewhere in C:\Users\XXX\AppData\Local\Temp\{UUID here}\
in which all writes just fail.
Just use the SYSCONF path + ".tmp" for the temporary file name.
These vertex formats enable all attributes. Inactive attributes are set
to offset=0, and the smallest type possible. This "optimization" stops
the NV compiler from generating variants of vertex shaders.
This add support for SD protocol 2 while staying compatible with protocol 1.01.
Most of this is quite hacky, but it seems to be working well.
The original implementation was quite confusing, so I didn't touch most of the stuff I did not understand.
This makes the EGL interface select OpenGL|ES contexts over "desktop"
OpenGL ones.
Possibly not useful for anyone outside my own debugging, but you never
know
It's not particularily useful to list the platform here,
and these kinds of messages that use words as parameters
are more likely to be mistranslated than the average string.
Same as the previous commit, except I'm copying strings
in the other direction because the DolphinWX variants
of these strings could use some improvement.
The spec says it should have an EXT not OES suffix, as it's enabled as
an interaction with GL_EXT_multi_draw_arrays.
On some drivers GetProcAddress() returns NULL, which causes the
GLExtensions init to fail
This 'happened' to work if GetProcAddress() doesn't return NULL on missing
functions (as allowed in EGL) - as the function appears to never be called so
this would not have been noticed.
Mesa also (incorrectly?) exports the EXT version, so this would all
happen to work there, but appears to be contrary to the spec.
This invalid prefix even ended up in the upstream khronos registry, the
issue was reported here:
https://github.com/KhronosGroup/OpenGL-Registry/issues/81
The section is 0x461 bytes long, not 0x460. The config data is also now
initialised to zero to avoid garbage being written to the SYSCONF.
Because our handling has been wrong forever, we discard older BT.DINF
section backups as using them would result in the section being the
wrong size / incomplete again.
It turns out that the last byte of array entries isn't unused (as we
thought); instead, it looks like it's actually part of the main data,
and the length stored next to the name is in fact the length minus one.
Getting it wrong and always storing a null byte in there won't affect
most entries (since the last byte is zeroed most of the time), except:
- IPL.NIK: the length is stored in the last byte, and it must be kept.
- BT.DINF: u8 unknown[0x45] should be another Bluetooth device entry.
- Possibly other unknown affected entries.
I don't know who thought it would be a good idea to put the Wiimote
connect code as part of the Host interface, and have that called
from both the UI code and the core. And then hack around it by having
"force connect" events whenever Host_ConnectWiimote is called
from the core...
BluetoothEmu had its own bdaddr_t type which is a old style C struct
and typedef, which makes comparisons and copies a bit ugly.
On the other hand, BTReal had its own btaddr_t type using std::array.
To make things very slightly nicer, this commit changes the Bluetooth
code to use a single type (std::array<u8, 6>) for all BT addresses.
Imports/exports don't always use the title key. Exporting a title and
importing it back uses the PRNG key (aka backup key handle or key #5),
not the title key (at all).
To make things even more fun, some versions of IOS have a bug that
causes it to use a zeroed key instead of the PRNG key. When Nintendo
decided to fix it, they added checks to keep using the zeroed key only
in affected titles to avoid making existing exports useless.
(Thanks to tueidj for drawing my attention to this.
I missed this edge case during the initial implementation.)
This commit implements these checks so we are using the correct key
in all of these cases.
We now also use IOSC for decryption/encryption since built-in key
handles are used. And we now reject any invalid common key index,
just like ES.
Core::PauseAndLock requires all calls to it to be balanced, like this:
const bool was_unpaused = Core::PauseAndLock(true);
// do stuff on the CPU thread
Core::PauseAndLock(false, was_unpaused);
Aside from being a bit cumbersome, it turns out all callers really
don't need to know about was_unpaused at all. They just need to do
something on the CPU thread safely, including locking/unlocking.
So this commit replaces Core::PauseAndLock with a function that
makes both the purpose and the scope of what is being run on the
CPU thread visually clear. This makes it harder to accidentally run
something on the wrong thread, or forget the second call to
PauseAndLock to unpause, or forget that it needs to be passed
was_unpaused at the end.
We also don't need comments to indicate code X is being run on the
CPU thread anymore, as the function name makes it obvious.
The region mismatch check that we used can give false positives.
Skipping the check won't lead to any harm - games will ignore
save files that have a non-matching fourth game ID character.
According to http://scanlines16.com/en/blog-3/retro-gaming/game-cube/gamecube-korean-master-list/,
Korean GC releases use the following country codes:
- E or W for games in English
- K for games in Korean
- Unknown value for games in Japanese (my guess is that they might
have made the discs bit-for-bit identical to Japanese releases
because the regions of these games are already set to NTSC-J)
As far as I know, the GC has no Taiwanese releases, which is what
the W country code is used for on the Wii. But I could be wrong.
A small note: The country_byte == 'K' check in the code isn't
actually necessary as long as RegionSwitchGC returns NTSC_J
for 'K', but I thought it would be better to not rely on that.
The county code isn't 100% reliable for detecting the region.
For instance, some games released in Korea have the country
code E even though they're region-locked to NTSC-J consoles.
This commit makes the GC disc region detection match the Wii
disc region detection (apart from the region value being in
a different place on the disc).
Pierre Bourdon