Skip to content

add IPreferences Interface#10830

Open
Hanmac wants to merge 2 commits into
masterfrom
iPreferences
Open

add IPreferences Interface#10830
Hanmac wants to merge 2 commits into
masterfrom
iPreferences

Conversation

@Hanmac

@Hanmac Hanmac commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Preferences Interface for #10827

@MostCromulent i'm not 100% sure if the YieldController changes work that way
or if i need to change the PlayerControllerHuman to IGameController
because NetGameController calls new YieldController(null);

but there are still some methods unimplemented for IGameController

(i can split up the YieldController changes into a different PR)

@MostCromulent

Copy link
Copy Markdown
Contributor

Claude says it will break multiplayer yields in this form:

There are two separate things here — the behavior change the refactor introduces, and the PlayerControllerHuman → IGameController question Hanmac raised. Splitting them out:

1. The regression (independent of the question)

Converting toggleAutoPassNoActions() / toggleAutoPassOrStopAll() from static (taking IGameController) to instance methods routed through owner changes behavior for network clients.

The originals took an IGameController because the toggle is a controller-level dispatch, and ctrl.setYieldPref(...) is polymorphic:

  • PlayerControllerHuman.setYieldPref → handles it locally
  • NetGameController.setYieldPref → sends it to the host over the wire (SetYieldPref envelope)

On a network client, getGameController() returns the NetGameController, whose YieldController is built as new YieldController(null)owner == null is the sentinel for the client's controller (no local PCH). So the instance version hits if (owner == null) return false; and returns early: the local pref isn't flipped and the SetYieldPref wire message isn't sent. The runtime toggle (P / menu mid-game) becomes a no-op on clients, and the menu checkbox doesn't update. The host-side proxy that decides auto-pass for that player reads YIELD_AUTO_PASS_NO_ACTIONS from its prefOverrides, populated only by inbound SetYieldPref/seed envelopes, so without the send the host keeps its own value. (Initial state still propagates via seedYieldStateOnHost at game start — it's specifically the runtime toggle that's affected.)

This is true regardless of how the owner type question is resolved — it comes from routing through owner (null on clients) instead of the controller.

2. The question: should owner become IGameController?

That change alone wouldn't fix the above, and it has a cost. Beyond the toggle, YieldController uses several PCH-only methods — getGui, getLocalPlayerView, getGame, getLobbyPlayer, isRemoteClient — none of which are on IGameController. Promoting owner to IGameController would mean adding all of those to the interface, which NetGameController (no local game state) can't meaningfully implement.

Options

  1. Keep the two toggle* methods static taking IGameController (revert just that slice). They don't need the instance prefs field — a UI toggle targets the local user's FModel pref, which is what they currently do. owner stays PlayerControllerHuman.
  2. Keep the instance style but give YieldController a separate IGameController dispatch reference (distinct from owner), with NetGameController passing this. The toggle routes through that reference; owner stays PlayerControllerHuman for the game-state methods.

@Hanmac

Hanmac commented Jun 2, 2026

Copy link
Copy Markdown
Contributor Author

reverted the YieldController for now

@allentiak

allentiak commented Jun 2, 2026 via email

Copy link
Copy Markdown
Member

@Hanmac

Hanmac commented Jun 2, 2026

Copy link
Copy Markdown
Contributor Author

You have already said that. I'm asking why splitting the functionality across the abstract class and the interface. I still don't see any reason for that, and none of your previous answers explain that single, crucial point. Would you be so kind as to please explain why? (Please refer to my earlier long answer as for why I'm asking this.)

Because the interface can't know how the Data is stored
(If at all)

For example, see Java HashMap

public class HashMap<K,V>
extends AbstractMap<K,V>
implements Map<K,V>

The interface has the methods list
The AbstractMap most of the implementation
And HashMap the final part to make it work

@allentiak

allentiak commented Jun 2, 2026 via email

Copy link
Copy Markdown
Member

@allentiak

allentiak commented Jun 2, 2026 via email

Copy link
Copy Markdown
Member

@allentiak

Copy link
Copy Markdown
Member

I cannot approve it myself since I don't have write access to the repo.

@Hanmac Hanmac marked this pull request as ready for review June 8, 2026 07:37
@Hanmac Hanmac requested review from Jetz72 and tehdiplomat June 8, 2026 07:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants