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 invisible units #1604

Closed
wants to merge 9 commits into from

Conversation

simon33-2
Copy link
Contributor

Builds on #1599

The major reason for this is to allow a universal scramble relatively easily by putting an airstrip in every territory.

@ron-murhammer
Copy link
Member

@simon33-2 I thought invisible units such as factories were already being done in some maps? Do you have any threads or issues to reference on other folks looking for this feature? Have you gotten anyone else to test this?

@simon33-2
Copy link
Contributor Author

I'm not aware of any maps with invisible factories? All this change does is make it possible to suppress the error for missing image. Perhaps they just live with the error in those maps.

@simon33-2
Copy link
Contributor Author

Many people are interested in the universal scramble and the invisible units is a quick (and slightly dirty) way of doing it. Do the invisible factories just give missing image errors?

To be honest, this is a nice to have for me but I thought it was worth a try.

@DanVanAtta
Copy link
Member

@simon33-2 there are a couple of items introducing some difficulty in accepting the updates as proposed:

  • mixture of major elements. We can review the work here, but there are 2 or 3 different functionalities being introduced, and you'll have to work through them all before anything can be merged in.
  • testing needs to be more significant and obvious (preferably fully automated). This is not an easy code base, and it's really easy to introduce major errors. Ideally there would be examples of XMLs and screenshots of the new invisible unit feature added. For a new feature it is generally better to discuss it first via an issue, otherwise there is risk of it not being accepted right away or at all sometimes.

@DanVanAtta
Copy link
Member

Recommendations for moving forward:

  • do some git foo to pull the updates into independent changes
  • avoid requiring a major number upgrade
  • open an issue to highlight the invisible unit feature and how it will be used. (for context: my concern is that we have many incomplete or fully unused features in the codebase. The complexity of it I view as also a number 1 problem, my inclination then is to verify that the features we do add are at least done correctly and complete.)

@simon33-2
Copy link
Contributor Author

Hi @DanVanAtta
I have sympathy for your viewpoint today. Re: 1.9.1 version, that change was picked up from the other PR.

I'm going to close this issue off now so we can just talk about the much more important issues in #1599.

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