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
Make empires remember the enemies they saw in battle. #29
Conversation
This is relevant if they have stealth or if all ships that could see them are destroyed.
Do we consider the issue addressed by this patch actually a bug, or is this more like an enhancement (just want to know how to label it)? |
Not 100% sure what it's doing, so checking: is it making it so that if, say, I lose a scout to an Asteroid Snail before I've researched Active Radar, I'll see a scanlined snail on the map and the current "Unknown" in the combat report will instead say snail? If that's what it's doing, I've always personally considered it a bad thing, but had assumed it was working as intended, so it's probably an enhancement, but could be a bugfix depending on previous design intent. |
I consider it a bug. And yes, Mat, your scenario is an example that this would cover. |
Anything preventing this from getting merged? Has anyone tested if that patch works as intended? |
I still don't like it and want to re-implement it differently. Posting it as a pull request doesn't change that. |
That sounds pretty final - in that case, shall we close this PR? |
This PR does work for it's purpose, though my initial testing showed a couple rough edges which I noted in the forum thread (though they could have been a glitch from my patching). Geoff, in the forum thread you previously seemed to object to trying to resolve your objections, saying "and I don't want to make this type of change right before a release"; that is why I asked Mitten to open up this PR now, so we could continue the discussion and get things resolved. It also wasn't clear to me you Geoff, were necessarily planning to re-implement this or just hoping someone else did. It looks to me like some small changes could be done to resolve the concerns you previously mentioned, I'll experiment with them and post results. If someone else implements a different solution that gets chosen over this then, sure, this PR should be closed at that time. Until then, I think it should stay open. |
After commit 84d07d8, can this PR finally be closed? |
Yes, right. |
This is relevant if they have stealth or if all ships that could
see them are destroyed.
I made sure that this compiles, but do not have time to make sure it still works.
It did merge cleanly, though, and I don't see any relevant changes in the associated code that should have broken it since it was made.