Skip to content
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

Add HPF overlay controls to observer chrome #20436

Merged
merged 2 commits into from Dec 12, 2022

Conversation

dragunoff
Copy link
Contributor

The controls associated with the "hpf" debug command were only visible in player context.

@PunkPun
Copy link
Member

PunkPun commented Nov 4, 2022

Common UI is used for the SDK, it's would be better to follow the convention and use common for RA and delete RA specific file rather than the opposite

@dragunoff
Copy link
Contributor Author

Common UI is used for the SDK, it's would be better to follow the convention and use common for RA and delete RA specific file rather than the opposite

How is it used in the SDK? If you mean other mods referencing it then it's a downstream problem. There's no point in keeping an unused file around as it cannot be tested.

@PunkPun
Copy link
Member

PunkPun commented Nov 4, 2022

I'm not suggesting keeping unused files, I'm suggesting using common for red alert, as it's already how we handle most of red alert and tiberian sun UI

@dragunoff
Copy link
Contributor Author

I'm not suggesting keeping unused files, I'm suggesting using common for red alert, as it's already how we handle most of red alert and tiberian sun UI

Yes, sure but then this doesn't help downstream mods at all as they will get a file that is tailored to RA but named "common". And we mess up the git history for no good reason.

@dragunoff
Copy link
Contributor Author

Anyhow, I don't care that much which file remains as long as we get rid of unused ones.

@PunkPun
Copy link
Member

PunkPun commented Nov 9, 2022

#20079 (comment)

@dragunoff
Copy link
Contributor Author

#20079 (comment)

@PunkPun I'm against using a "common" file here because it's not common at all - the ingame UI is slightly different for each mod. So downstream mods linking to a file tailored for another mod probably won't get good results. And in the end it's a downstream problem - chrome yaml files change all the time and it's downstream mods responsibility to make sure they keep up with updates. If you insist on having a common file for ingame-observer then I'd suggest using the one from TS or D2k as the diff there is smaller (the one for RA just differs too much to the current common one).

@PunkPun
Copy link
Member

PunkPun commented Nov 18, 2022

Let's go with TS then. We should investigate how many other UI pieces don't have a common variant

@abcdefg30
Copy link
Member

I disagree that we should have a common file just for the sake of it. (While in reality it is not common but just used by one mod.) Nothing is stopping downstream mods from copying the TS or RA file if they need it.

@PunkPun
Copy link
Member

PunkPun commented Dec 8, 2022

Tested all 4 mods, as a player and a spectator

@abcdefg30 abcdefg30 merged commit e60f7bb into OpenRA:bleed Dec 12, 2022
@abcdefg30
Copy link
Member

Changelog

@dragunoff dragunoff deleted the feature/hpf-overlay-observers branch December 13, 2022 08:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants