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

Allow cloaked units under player control to have SELECT cursor on it when about to select #20

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

Conversation

chaserli
Copy link
Member

@chaserli chaserli commented Mar 10, 2024

Basically at this point it's useless to do all these checks.
What had the program done?

if(!this->IsArmed() || !this->Owner->ControlledByCurrentPlayer() || this->Owner==pObject->GetOwningHouse())
    if((this->Spawned || !this->ControlledByCurrentPlayer)
          && pObject->CanBeSelected() &&..)
         return Action::Select;
 
 return Action::None;

This is the very end of that function it's not sufficient to only let "me" select pObject if it belongs to this->Owner which is known to be controlled by me. It is also possible that "I" (the controller of this) can only control the owning house of pObject but is not it. In that case you gotta let me select it too

Copy link

github-actions bot commented Mar 10, 2024

Nightly build for this pull request:

This comment is automatic and is meant to allow guests to get latest nightly builds for this pull request without registering. It is updated on every successful build.

@chaserli chaserli requested a review from Belonit March 10, 2024 07:58
@chaserli chaserli marked this pull request as draft March 10, 2024 09:48
@chaserli chaserli marked this pull request as ready for review March 12, 2024 02:35
@Belonit
Copy link
Member

Belonit commented Mar 13, 2024

@chaserli If you don't mind, I'll get back to this PR in a couple of weeks. There are a few other things I'm busy with right now

@chaserli
Copy link
Member Author

@chaserli If you don't mind, I'll get back to this PR in a couple of weeks. There are a few other things I'm busy with right now

sure not, you could even work on #21 before this one

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.

2 participants