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

Issue 142: FTMemberMarkers fix JIP issues #156

Merged
merged 2 commits into from
May 19, 2018

Conversation

shadow-fa
Copy link
Collaborator

Previously this component was drawing white markers for JIP players.
This was because fn_LocalFTMarkerSync was only "syncing" the values if they had changed from the group leaders perspective.
Additionally it would only sync if the group leader did not disconnect.

I'm not sure if this is the best solution to the problem, but I'm open to discuss any alternatives.

See also issue #142.

@shadow-fa shadow-fa added this to the 3.5.2 milestone Mar 23, 2018
@shadow-fa shadow-fa requested a review from SamLex March 23, 2018 18:49
@shadow-fa shadow-fa mentioned this pull request Mar 23, 2018
@shadow-fa shadow-fa changed the title FTMemberMarkers fix JIP issues Issue 142: FTMemberMarkers fix JIP issues Mar 25, 2018
@SamLex
Copy link
Member

SamLex commented May 19, 2018

Can't this component be significantly simplified now that assignTeam is global and with #163?

Quite a lot of the logic in this component (and what caused the JIP issue) is the syncing of the team colours using a variable on each unit, all of which is now redundant. So that logic could be removed and the markers just drawn based on the (now global) assignedTeam.

@shadow-fa
Copy link
Collaborator Author

shadow-fa commented May 19, 2018

It already is simplified because the syncing is gone.
What still has to be done is storing the colour in a variable for each unit, because at some points in the game (e.g. when the unit is controlling a UAV), we can't get the colour from the command assignedTeam because it would return nil. This would lead to the problem, that some units would sometimes have a different colour.

@SamLex
Copy link
Member

SamLex commented May 19, 2018

Yeah, I should look more carefully before commenting.

Also, doesn't this run (unnecessarily) on the server (no hasInterface check)?

@shadow-fa
Copy link
Collaborator Author

It should run for every unit that has an interface, including the server if it isn't a dedicated server.

@SamLex
Copy link
Member

SamLex commented May 19, 2018

Yes, but doesn't it currently run even on machines without an interface?

@shadow-fa
Copy link
Collaborator Author

You're right, I got it confused with another PR.
This (see last commit) is what you meant, right?

@SamLex SamLex merged commit e6cbd51 into folkarps:dev May 19, 2018
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.

2 participants