Skip to content

Conversation

@YoshiRulz
Copy link
Member

Per today's conversation on Discord.

@YoshiRulz YoshiRulz added the Meta Relating to code organisation or to things that aren't code label Nov 24, 2025
@Morilli
Copy link
Collaborator

Morilli commented Nov 24, 2025

I personally don't like making things that are actively used disallowed by default and have them require a #pragma warning ignore to use (not just here, in other places as well). It's very awkward to use and makes it feel wrong, even if the intended use case is valid.

Making it the standard to have every single use case pragma warning ignored cannot be sensible.

I explicitly added the RomPath property argument because it was necessary to implement functionality that couldn't be implemented without it. If this causes non-determinism because things suddenly depend on specific file names (or worse, full paths) to work then perhaps add a doc comment to the property and monitor usages, but don't outright disallow its use.

@YoshiRulz
Copy link
Member Author

YoshiRulz commented Nov 25, 2025

In general I agree.
On the particulars of this property, I would disagree, and maintain that it needs to be phased out. I've previously said (IIRC in one of eien's core PRs) that cores must be pure with respect to the host filesystem, and they can't assume rom files even have a path at all because roms may be loaded from a network stream. edit: Also as CPP points out below, the string may be a HawkFile archive path.

@CasualPokePlayer
Copy link
Member

CasualPokePlayer commented Nov 25, 2025

I don't particularly like this implementation, ideally the warning should alert the developer that there are alternatives available (Extension and Game.Name should be used if those are what RomPath ends up getting used for), and RomPath can't necessarily be used raw for file access, since it may actually be a file in an archive, therefore the path is invalid as it has a | (granted this is probably more a doc update if anything, maybe this warning too should yell at the developer for every inserting the prop into a Path.* API since such may fail due to the |). RomPath being used should be used extremely sparingly in any case, and never, ever be sent into the waterbox (again Extension and Game.Name are available).

@YoshiRulz
Copy link
Member Author

Does your IDE not show it?

[Obsolete("you should never have to use this property, Game.Name and Extension should be sufficient")] // not deprecated, do not remove

@Morilli
Copy link
Collaborator

Morilli commented Nov 25, 2025

cores must be pure with respect to the host filesystem, and they can't assume rom files even have a path at all because roms may be loaded from a network stream

As a matter of fact some established functionality DOES depend on the host filesystem, for example msu1 support for bsnes/snes9x. There might be a way to implement this in a way that it doesn't depend on the filesystem and break compatibility with every other emulator and make it unusable for every casual user (or we could drop support outright), but I don't think that's a great alternative.

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

Labels

Meta Relating to code organisation or to things that aren't code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants