Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adding Opera (3DO) as emulation core in Bizhawk #4264
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
base: master
Are you sure you want to change the base?
Adding Opera (3DO) as emulation core in Bizhawk #4264
Changes from 82 commits
c82506a
fc8fa94
b90d6f6
bc213ed
63a5ac6
d3be088
e7aeebb
98f2191
57af521
face41a
400f4b9
935a4d1
4a507cb
d85bf7e
5994501
2cef3f1
b09d17e
6866911
cce05d7
4c97d6d
707f890
8f6ffe5
95b7a93
3adcba0
6445eea
3fb18a6
634fcd7
14d77aa
6375152
339b3fa
62922cb
d141155
59c1f7c
3937334
24f496b
4505b66
6523c94
1201035
26f9501
95dec71
db9d212
0e5030c
7a4be1d
58dc832
f74d7ec
4b68111
02e8313
158f91e
ad3636a
4fcb12b
90f0673
5f3ed39
833663d
5c31c3a
f1222c2
543e1e5
fe75a5c
e0f6f05
f93584c
633c2c3
fba0d59
39f566d
710d0ba
fecbc9e
b7260d1
0dcdcaf
7fa5f53
2669fda
b503e08
aa88205
ea69752
3abad35
683ae9b
6e24c6b
0de3211
06a3684
d286bd0
496ca8c
ed08e48
701931a
f3ca025
adc640c
12f7696
26344cc
172de50
fc8871a
57c8d64
f7b3e52
0a9dd92
06596ff
3d69272
dfd9d92
c2e2fce
2ed6f42
f3058be
11de17d
3b77bd1
0b8c736
7d72f4e
4294022
62923a2
07e0a09
65e79e3
600d9a9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 there a reason these are all
int
s instead of a type more closely resembling their data range, likebool
?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.
I did try using
bool
but I keep getting false at the core side, even when I send trues from the bk side. I looked high and low for a discrepancy in the apis, but everything looks correct.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.
When using bool, was the core actually using bool too, or just ints? The struct sizes need to match up.
Also the
Compatibility = true
would also probably cause issues regardless if you did that, as that would go under "normal" marshalling rules (i.e. the struct is converted into one with 4 byte bools instead of just directly pinning the original struct). You generally shouldn't use Compatibility = true unless you need "normal" marshalling as that slower.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.
I tried again, making sure again that the structs coincide. You can find them here:
[BK Side]
https://github.com/SergioMartin86/BizHawk/blob/b4836176a8df5d2eba2d77598412036767840d8d/src/BizHawk.Emulation.Cores/Consoles/3DO/LibOpera.cs#L58
[WBX side] https://github.com/SergioMartin86/BizHawk/blob/b4836176a8df5d2eba2d77598412036767840d8d/waterbox/opera/bizhawk.hpp#L13
The values are simply not passed around:

Perhaps you will see the discrepancy -- I just don't see it
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.
It's probably got something to do with the bullshit default bool marshalling rules that nobody wants, although
BizInvoke
should usually get rid of that...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.
Looks like the bool
isReset
is getting tagged onto the baseFrameInfo
before the structs.Using https://github.com/SergeyTeplyakov/ObjectLayoutInspector this is the actual layout:
Full managed memory layout
Having any of the struct's fields be
bool
makes it no longer align for reasons I cannot tell you.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.
What's the state of this discussion?
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.
I think the problem is that bool types don't count as blittable types for whatever garbage legacy reason in C# which is most likely the reason using bool in any struct at all no longer guarantees struct layout. So as unsatisfied as I am with the conclusion I say leave it as is.
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.
Buried in "obsolete" documentation is
[MarshalAs(UnmanagedType.U1)]
(though here it says to useUnmanagedType.I1
instead), would that solve the blittability problem?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.
I don't think so. Applying atrributes to every bool would only change the layout in interop which we're not using here.