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

Unhide Alliance tab for teammates #346

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JamieNyanchi
Copy link
Contributor

@JamieNyanchi JamieNyanchi commented Dec 26, 2022

The purpose of this change is to unhide the Alliance tab in the DiplomacyActionView when playing on a team. The game normally hides the Alliance tab when you're on the same team as the leader you're currently viewing. This is confusing behavior though as you are still able to form alliances with other leaders on your team. The information behind the Alliance tab isn't meant to be a secret to the player, so it can be unhidden without negatively impacting any aspect of the game. The associated commit simply overrides the base game's AddIntelAlliance function and removes the team check at the beginning.

I believe this change falls within the scope of CQUI as having access to this information can help you more easily and quickly manage your alliances since you don't have to try to keep track of how many alliance points you have, for instance.

@Infixo
Copy link
Contributor

Infixo commented Apr 6, 2023

How does the vanilla game UI work here? Does it show the info or not?

@JamieNyanchi
Copy link
Contributor Author

In the vanilla game, if you open up the diplomacy action view with another player who is on the same team as you, it will hide the alliance tab. If the other player is not on the same team as you, it will show. So, as it is right now, CQUI is doing vanilla behavior. I updated the PR to better show the part of the overridden function I actually modified. I only commented out the the if statement at the beginning. The rest of the function is unchanged.

As for my justification for making this change, I just don't think it makes sense to hide this tab. Maybe at some point it was intended for teammates to not form alliances with each other, but in any case, they are able to form alliances just fine in the vanilla game. Unhiding this tab doesn't show any secret information either as the tab only shows the current status of your alliance (current alliance level and alliance points).

@Infixo
Copy link
Contributor

Infixo commented Apr 6, 2023

I am not willing to change vanilla behavior especially in regards to multiplayer. We must be sure that this is actually needed and vanilla behavior is erroneous.

@JamieNyanchi
Copy link
Contributor Author

JamieNyanchi commented Apr 6, 2023

Hmm, the best evidence I can provide that this may be erroneous is this post from the Civfanatics forums:
https://forums.civfanatics.com/threads/1-0-0-290-reproducible-alliance-status-not-visible-in-multiplayer-teams-1-0-0-314-still-exists.643739/

If I understand correctly, user dshirk is a Firaxis employee who was also a lead producer for Civ 6. His response to this makes me think this was erroneous and it was supposed to be changed. However, it apparently never made it to the released version and I can't find why. In any case, his message seems to imply the current behavior is not intended.

To provide a bit of extra clarity just to be safe, this doesn't change whether or not you can make alliances. You've always been able to do that on teams (once you've gotten the related civic of course). This just changes the visibility of the tab that shows you the status of your alliance. This is visible with everyone who isn't on your team, so it just seems strange for it to not be visible here. You could even manually keep track of the progress yourself right now too, so it's not necessarily secret information.

All that said, I do understand the concern with making this change if it actually was intended to be the way it is. That's why I think the post on the Civfanatics forum is the best evidence that it isn't intended the way it is.

@the-m4a
Copy link
Collaborator

the-m4a commented Apr 7, 2023

I agree with Jamie - this seems like a pretty straightforward fix that's not breaking game rules. I am fine with this change.

@the-m4a
Copy link
Collaborator

the-m4a commented Apr 7, 2023

Approving, but will wait to merge pending comment from Infixo if desired to do so

Copy link
Contributor

@Infixo Infixo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, the change is simple, but it doesn't follow guidelines to making things in one file if possible. In this case the code should be placed in _CQUI.lua file with proper checks for expansions (g_bIs... etc.) Actual _expansion.lua files should stay clean and just contain includes.

Overrides the AddIntelAlliance function in the DiplomacyActionView file to remove hiding the Alliance tab if the player is on the same team as the leader they're viewing
@JamieNyanchi
Copy link
Contributor Author

JamieNyanchi commented Apr 7, 2023

Ah, so that's the preferred coding system here after all! I honestly wasn't sure which was preferred. I'll keep that in mind for the future! That probably also means I need to fix the other PR I have open... Anyway, I have moved the code so it's only in the base file. If I'm not mistaken, this should be okay without expansion checks as this function is only called in Rise and Fall and Gathering Storm (and is identical in both cases). The local variable that is added is also only used by this function.

@Infixo
Copy link
Contributor

Infixo commented Apr 7, 2023

Yes :) Just make sure it is called when necessary :)
@the-m4a spent lots of time cleaning those files :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants