yairm210 / Unciv

Open-source Android/Desktop remake of Civ V
Mozilla Public License 2.0
8.34k stars 1.56k forks source link

No indication that oil well in water requires refrigeration #5537

Closed qbcrunch closed 2 years ago

qbcrunch commented 2 years ago

Platform Platform Kindle Fire 10 (11th Gen) FireOS ver 7.3.1.9 with Google Play Store side loaded

Version 3.17.8 from Google Play Store (latest available at time of posting)

Describe the bug Describe the bug There is no indication anywhere that oil wells in water require refrigeration. There is a symbol on the refirgeration bubble in the civilopedia but there is no text in there to indicate what that symbol might mean or that it is require for water-based oil wells. This stems from the responses to the previously closed issue #5525.

To Reproduce Steps to reproduce the behavior:

  1. Go to '...'
  2. Click on '....'
  3. Scroll down to '....'
  4. See error

If game data is required, please add it here. From the menu, choose save game (or map), copy to clipboard, and paste here between the backtick blocks (or, if no such save is required, you may delete this):

Saved game ``` ```

Expected behavior Text block in the civilopedia under refrigeration tech to indicate "Allows oil wells on water tiles" Screenshot_20211022-101940

Screenshots If applicable, add screenshots to help explain your problem.

Additional context Add any other context about the problem here.

qbcrunch commented 2 years ago

Screenshot_20211022-105340

alextfish commented 2 years ago

I thought the tech for Biology used to say something like "Improvements enabled: * Oil well (Cannot be built on Coast until Refrigeration is discovered)". The Civilopedia entry for Oil well does still say that, but I can understand that nobody reads that.

(I really wish the tech tree had hyperlinks through to other civilopedia entries like tile improvements. But that's a separate issue.)

SomeTroglodyte commented 2 years ago

@xlenstra - could/should that unique be rewritten to use conditionals?

xlenstra commented 2 years ago

I've thought about that when working on the resource unique enumification PR, and decided against it at the time. The logical way to write the unique would be: "Cannot be built on [Coast] tiles \<before researching [Refrigeration]>", and doing it like that also unifies it with "Cannot be built on [Bonus resource] tiles". My main reason was to not do this was that it should check whether a certain condition does not apply, and the framework isn't built to handle that.

One way around that would be rewording it to "Can only be built on [Coast] tiles \<after researching [Refrigeration]>", but then the unique without the conditional would read "Can only be built on [Coast] tiles", which means something different. Wording it as "Can only be built on [Land] tiles \<before researching [Refrigeration]>" would be technically correct, but is also confusing to a certain degree. Furthermore, that way of writing doesn't work for the "Cannot be built on [Bonus resource] tiles", so all advantage of merging these is gone. If you have a good wording for the unique, it might be worthwhile, but I haven't found one yet.

qbcrunch commented 2 years ago

I just wanted to add, I played a lot on much older versions which didn't have this requirement. As I received various updates of Unciv, the tech tree and what get enables where has changed drasitically, sometimes multiple times through a game that I'm playing through over a short period of time. The only way I was able to figure what was now required for many of my preferred wonders, improvements, etc was to go through the entire tech tree.

The currently displayed text in the UI for Biology states: "... Reveals Oil on Map." and "Tile improvements enabled: Oil Well" there is no other text displayed after this. Unfortunately, I can't screen cap the Civilopedia page entry as screen capture on this device covers the bottom line with the back, home, recent android navigation bar. So, as an end user, there's really no indication that refrigeration is required for the water tiles. Even a mention on the Refrigeration entry as stated in the intial entry on this thread would be sufficient. If these entries are autogenerated based on the conditionals you are mentioning, then I understand that could be a significant and difficult task.

SomeTroglodyte commented 2 years ago

Exercise: Replace the description label bottom left of the tech picker with a 'pedia render - easy enough, but too much for the cramped place down there (but the links work). Can't screenshot at the moment.

Screenshot ![image](https://user-images.githubusercontent.com/63000004/138867987-c20ad246-5ce4-4346-9e1b-4343d218a3bb.png)

Some tweaking might make that a bit better to use (drop era via reintroducing a "for the picker" parameter, reduce v pad, reduce icons a bit), but that's limited.

Another option: Leave the label a Label, but link it to the tech's pedia entry (no direct way back mind you).

Of course, that's only accessibility, not information completeness so far. Let's see why the button itself knows the Oil Well is related..

(And I still think the Oil Well on Land and the Offshore Platform should be distinct objects)

SomeTroglodyte commented 2 years ago

The button itself paints the Oil Well icon because: it.techRequired == techName || it.uniqueObjects.any { u -> u.allParams.contains(techName) - smells a little regression-prone - but also emphasizes a conditional would be neat...

Back to "Cannot be built on [Coast] tiles <before researching [Refrigeration]>" -> why not? The un-conditionalized unique works as negator only because the client-side query chooses to interpret 'true' as 'prevent'. Having the conditional disable the unique if false is the same structurally as other uniques. So tech researched -> conditional false should work???

That still leaves the problem that displaying the improvement's unique on the tech, readable, not only the icon, would need some kind of linguistic inversion -> describing the inner working would sound like "(Refrigeration) disables "Cannot be built on Coast" for the Olli Well improvement"? Ugh. We'd be back to a hardcoded description for the specific unique/condition combo to make it sound nice. Workaround: Add a "See-also" mention only?

xlenstra commented 2 years ago

At this point I think we're trying to create a band-aid that is almost as large as the original problem. What we want is two different improvements that both connect oil to your empire, 'oil well' and 'offshore platform'. Allowing the oil well to be built on water only with the refrigeration tech was, as I understood it from avdstaaij, a quick fix to implement it, and never meant as a long-term solution. When these two improvements are both added, any work with conditional uniques or hardcoding is no longer necessary. Adding the offshore platform also solves the problem with users not knowing the difference, as the oil page in the civilopedia will display two improvements, and the tech tree also contains a reference to two improvements. Maybe at this point we can introduce 'may only be built on [Land] tiles' and 'may only be built [Water] tiles' uniques for these two improvements to further clarify the distinction.

nsoranzo commented 2 years ago

Maybe at this point we can introduce 'may only be built on [Land] tiles' and 'may only be built [Water] tiles' uniques for these two improvements to further clarify the distinction.

Sorry to interject, I guess this part is equivalent to being built by a Worker vs Work Boat?

xlenstra commented 2 years ago

Maybe at this point we can introduce 'may only be built on [Land] tiles' and 'may only be built [Water] tiles' uniques for these two improvements to further clarify the distinction.

Sorry to interject, I guess this part is equivalent to being built by a Worker vs Work Boat?

In vanilla unciv that is indeed the case, but mods are allowed to have different units for doing this, so we can't directly check for the unit. Checking the type of tile they are on is then probably the easiest way to distinguish them, which is the reason I went for that

SomeTroglodyte commented 2 years ago

...was, as I understood it from avdstaaij, a quick fix...

TileResource.improvement: String? -> replace with improvements: List<String>, or with uniques on the left or right side - or dual landImprovement/waterImprovement fields, or?? Considering flexibility and automation performance I'd almost advocate the List over uniques. Anyway, not in my scope for today.