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

Move constructions to generic factory, fix some blueprint precalc bugs #1729

Merged

Conversation

olanti-p
Copy link
Contributor

@olanti-p olanti-p commented Jul 17, 2022

Summary

SUMMARY: Infrastructure "Move constructions to generic factory, fix some blueprint precalc bugs"

Purpose of change

Constructions:

  1. Enable copy-from for constructions
  2. Simplify implementation of new string_id<T> and int_id<T> that work in conjunction with single generic_factory<T>
  3. Clarify N/A UI note for constructions that don't require skills
  4. Allow player to build t_resin_hole_c same as similar resin terrain
  5. Enable text style checks for construction descriptions
  6. Reduce inclusions of construction.h header

Blueprint requirements autocalc:

  1. Fix tool duplication in requirements when list already contains replacing type (e.g. if list has fake_welder and welder, fake_welder would show up twice) Moved into Requirements: fix tool duplication when list already contains replacing type #1756
  2. Fix autocalc not following recipe chains beyond first 2 links
  3. Fix autocalc producing different results depending on order of definition of constructions in json files
  4. Fix autocalc silently skipping tiles that don't have construction recipes
  5. Fix autocalc producing nonsense requirements for some tiles (e.g. according to precalc you apply a paint chipper to empty tile, and then cover it with white paint, and get t_wall_w)
  6. Update blueprints with fixed requirements

Describe the solution

Constructions:

  1. Move constructions to generic factory
  2. Extract boilerplate code into macros in a separate header
  3. Replace note with No skills required
  4. Add corresponding construction recipe
  5. Change description to use translation class, and group constructions by a dedicated construction group id (port of Use class translation in construction and group construction by group id instead of description CleverRaven/Cataclysm-DDA#44566). Auto-generate construction groups for individual construction recipes to cut down on boilerplate. Use script to replace descriptions with group ids (included in "Additional context" section) Scrapped this idea, it doesn't play nicely with copy-from.
  6. Main offender was submap.h that included construction.h only to get definition of partial_con structure. Moved that structure into separate header, construction_partial.h, and made submap.h include that instead.

Blueprint requirements autocalc:

  1. Check if the list already contains replacing type before adding it.

  2. There were 2 problems here:

    • The function that was supposed to recursively look for construction recipes had a bug in it, so instead of doing recursive lookup it quit after first 2 steps.
    • There are construction recipes that form cycles (e.g. constr_window_taped and constr_revert_window_taped), and construction recipes that allow alternative ways of building object (e.g. constr_table and constr_place_table). As such, they form a directed graph with loops and parallel edges, and autocalc must have some deterministic way to decide on a single path through it.

    Solution here is to introduce "construction sequences". It's basically sequences of construction recipes that tell autocalc how to get from empty tile to desired terrain/furniture. To cut down on boilerplate, a construction sequence of 1 element is automatically generated for each construction recipe that starts with empty tile and results in a terrain/furniture, but only if there is only 1 such recipe that produces that specific terrain/furniture. This automates handling of trivial cases while forcing manual intervention for when it's unclear on which recipe(s) should be used.

  3. Hide accessor for raw construction recipes list, and instead build a sorted list on finalization and use that to iterate. The recipes are sorted by string ids, so iteration order becomes independent from definition order.

  4. Now that we have a concrete list of construction sequences, autocalc can tell for sure when it doesn't know how to build specific terrain/furniture and will show an error. The developer (or modder) then can decide on course of action:

    • Implement needed construction recipe(s) and/or construction sequence
    • Silence the warning by implementing an empty sequence, causing autocalc to skip that terrain/furniture
    • Silence the warning by turning off requirements check for that specific blueprint
  5. Since construction sequences now have concrete definitions, autocalc doesn't have to guess and pick some random solution that may not be a solution at all due to bug N2.

  6. Run tools/update_blueprint_needs.py and feed it debug.log produced after with the fixes.

Describe alternatives you've considered

  1. Some elaborate algorithm that would walk the recipe graph and return all possible options, than collapse them into single requirements list.
    • That'd be hard, and way out of original scope of the PR (moving constructions to generic factory)
    • I'm not sure whether our requirements system supports enough complexity to express all possible combinations
  2. Not porting the construction grouping PR? Copy-from is now a thing, so there's no need to de-duplicate description strings. On the other hand, having it be explicit now makes it possible to have different groups with same description.
  3. Getting rid of basecamps

Testing

Constructed a bunch of stuff, constructed a bunch of copy-from'ed stuff.
Manually calculated a few blueprints, compared with autocalc.

TODO list

  • Handle blacklisted construction recipes in sequences
  • Check autogeneration of construction groups for copy-fromed constructions Not needed anymore
  • Get rid of commented out code (leftover from prototyping)
  • Test some more
  • Write docs
  • Take a look over newly precalculated requirements in blueprints, possibly adjust them

Additional context

Yet another rabbit hole...

Also, here's the script to replace descriptions with group ids. It's an altered version from the original PR (CleverRaven/Cataclysm-DDA#44566) that doesn't produce groups that would have 1 element

Spoiler
#!/usr/bin/env python3

import glob
import json
import os
import re
import subprocess
import sys


def dump_json_and_lint(content, path):
    with open(path, 'w', encoding='utf-8') as fs:
        json.dump(content, fs, indent=2)
    json_formatter_name = glob.glob(
        'tools/format/json_formatter.[ec]*')
    assert len(json_formatter_name) == 1
    subprocess.run([json_formatter_name[0], path],
                stdout=subprocess.DEVNULL)


def main(argv):
    mod_dirs = {
        "data/core",
        "data/json",
    }

    with os.scandir("data/mods") as it:
        for entry in it:
            if entry.is_dir():
                mod_dirs.add(os.path.join("data/mods", entry.name))

    json_filename = re.compile("[^.].*\\.json")

    group_to_desc_map = dict()
    mod_to_groups_map = dict()

    for mod_dir in mod_dirs:
        mod_to_groups_map[mod_dir] = set()
        print("walking dir {}".format(mod_dir))
        for root, dirs, files in os.walk(mod_dir):
            for file in files:
                json_path = os.path.join(root, file)
                content = None
                if json_filename.match(file):
                    try:
                        with open(json_path, 'r', encoding='utf-8') as fs:
                            content = json.load(fs)
                    except Exception:
                        sys.stderr.write('Error parsing %r\n' % json_path)
                        raise
                if type(content) is list:
                    for j in range(len(content)):
                        obj = content[j]
                        if not (type(obj) is dict and
                                "type" in obj and obj["type"] == "construction" and
                                "description" in obj):
                            continue
                        group = re.sub(r'[^\w\d]+', '_', obj["description"].lower()).strip('_')
                        assert group not in group_to_desc_map or group_to_desc_map[group]["desc"] == obj["description"]
                        if group in group_to_desc_map:
                            group_to_desc_map[group]["count"] += 1
                        else:
                            new_group = dict()
                            new_group["desc"] = obj["description"]
                            new_group["count"] = 1
                            group_to_desc_map[group] = new_group
                            mod_to_groups_map[mod_dir].add(group)
                
        for root, dirs, files in os.walk(mod_dir):
            for file in files:
                json_path = os.path.join(root, file)
                content = None
                changed = False
                if json_filename.match(file):
                    try:
                        with open(json_path, 'r', encoding='utf-8') as fs:
                            content = json.load(fs)
                    except Exception:
                        sys.stderr.write('Error parsing %r\n' % json_path)
                        raise
                if type(content) is list:
                    for j in range(len(content)):
                        obj = content[j]
                        if not (type(obj) is dict and
                                "type" in obj and obj["type"] == "construction" and
                                "description" in obj):
                            continue
                        group = re.sub(r'[^\w\d]+', '_', obj["description"].lower()).strip('_')
                        assert group in group_to_desc_map
                        if group_to_desc_map[group]["count"] > 1:
                            new_obj = dict()
                            for k, v in obj.items():
                                if k == "description":
                                    new_obj["group"] = group
                                else:
                                    new_obj[k] = v
                            content[j] = new_obj
                            if not changed:
                                changed = True
                                print("updating {}".format(json_path))
                if changed:
                    dump_json_and_lint(content, json_path)

        if mod_to_groups_map[mod_dir]:
            groups_filtered = [ group for group in sorted(mod_to_groups_map[mod_dir]) if group_to_desc_map[group]["count"] > 1 ]

            if len(groups_filtered) > 0:
                mod_groups_json = [
                    {
                        "type": "construction_group",
                        "id": group,
                        "name": re.sub(r'\.$', '', group_to_desc_map[group]["desc"]),
                    } for group in groups_filtered
                ]
                dump_json_and_lint(mod_groups_json, os.path.join(mod_dir, "construction_group.json"))


if __name__ == "__main__":
    main(sys.argv[1:])

@olanti-p olanti-p force-pushed the misc-construction-refactoring branch 2 times, most recently from 901351a to 7bf4666 Compare September 12, 2022 11:21
@olanti-p olanti-p changed the title [WiP] Move constructions to generic factory, fix some blueprint precalc bugs Move constructions to generic factory, fix some blueprint precalc bugs Sep 12, 2022
@olanti-p
Copy link
Contributor Author

Ready, I think.

Many blueprints have become cheaper in terms of resources due to no longer requiring wacky semi-random construction steps, and many now take slightly longer to complete due to time now being properly accumulated from all steps.
Also many blueprints lost requirements for machete or scythe because precalc no longer tries to build dirt tile out of grass.

Copy-from is a bit awkward when it comes to requirements: construction must either use parent's reqs, or completely overwrite them. Not sure whether that's solvable until reqs themselves support copy-from.

Qrox and others added 24 commits September 17, 2022 23:55
(cherry picked from commit 46dd680875754839976f0238d1a3e64c824abc77)
(cherry picked from commit d0c62ab0f105052339c5bac6600a21d08f208beb)
(cherry picked from commit 4780fd5d7f4392d0bf8b22829497baf3821e066d)
(cherry picked from commit c9461c372aadf8ae8fe183b73bf45f848fddc301)
@olanti-p olanti-p force-pushed the misc-construction-refactoring branch from c5d6397 to 91652d1 Compare September 17, 2022 20:58
@Coolthulhu Coolthulhu self-assigned this Oct 5, 2022
@Coolthulhu Coolthulhu merged commit 63ec0b7 into cataclysmbnteam:upload Oct 5, 2022
@Coolthulhu
Copy link
Member

Being able to copy-from constructions is pretty huge.

tyzegt added a commit to tyzegt/CampsCostReduced that referenced this pull request Oct 6, 2022
@olanti-p olanti-p deleted the misc-construction-refactoring branch October 11, 2022 21:51
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