[18TS] Map and companies

[douglasfir]

Map and companies for 18TS, an 1825 inspired design by site user Martin2D. My implementation rips heavily from the 1825 implementation. Unlike 1825, there are only the 3 "units", no additional variants, so the setup scripts are significantly simpler.

[ollybh]

It's going to take a while to review a pull request that's this big. Here are some comments and questions on the first part of this PR.

[ollybh]

The add_entities method seems like overkill. You can simply use array concatenation.

Suggested change

	corps = []
	add_entities(corps, UNIT1_CORPORATIONS) if @units[1]
	add_entities(corps, UNIT2_CORPORATIONS) if @units[2]
	add_entities(corps, UNIT3_CORPORATIONS) if @units[3]
	corps
	corps = []
	corps += UNIT1_CORPORATIONS if @units[1]
	corps += UNIT2_CORPORATIONS if @units[2]
	corps += UNIT3_CORPORATIONS if @units[3]
	corps

[ollybh]

If the duplicate company definitions are all identical (I haven't checked the code to confirm this), you can use array addition and call uniq to get rid of any duplicates.

Suggested change

	comps = []
	add_entities(comps, UNIT1_COMPANIES) if @units[1]
	add_entities(comps, UNIT3_COMPANIES.reject { |c| comps.any? { |comp| comp[:value] == c[:value] } }) if @units[3]
	add_entities(comps, UNIT2_COMPANIES.reject { |c| comps.any? { |comp| comp[:value] == c[:value] } }) if @units[2]
	comps
	comps = []
	comps += UNIT1_COMPANIES if @units[1]
	comps += UNIT3_COMPANIES if @units[3]
	comps += UNIT2_COMPANIES if @units[2]
	comps.uniq

[ollybh]

This is a bit strange. You're using a hash, but the only hash value used is true, and you're just testing whether a hash key is present or not, which means your code wouldn't be able to tell the difference between @units[1] = true and @units[1] = false.

An alternative would be to use an array. Or a set if you're worried about duplicates.

@ units = ::Set[]
...
@units << 1 if @optional_rules.include?(:unit_1)
...
raise OptionError, 'Must select at least one Unit if using other options' unless @units.intersect?([1, 2, 3])

[ollybh]

Is this needed? Isn't it just giving more conditions to check later on?

[ollybh]

If you have multiple units selected then @units.keys.sort.map(&:to_s).join is going to give values like 12, 23 or 123 which aren't matched below.

(And, fyi, the map(&:to_s) call isn't needed, it will happen automatically when join is called).

[ollybh]

This is repeated in several other places.

[ollybh]

Are you trying to test how many units have been selected? That would be @units.size.

[ollybh]

Are these needed? 119 is a green tile with brown stripes, so should automatically be available when green tiles are available. Likewise 166 is a brown tile with grey stripes, so should become available at the same time as brown tiles.

[ollybh]

Obsolete code? There isn't a unit 4 mentioned elsewhere.

[ollybh]

Are the pre-printed hexes brown or sepia?

[ollybh]

You can use the spaceship operator here:

Suggested change

	@formed.find_index(a) < @formed.find_index(b) ? -1 : 1
	@formed.find_index(a) <=> @formed.find_index(b)

[ollybh]

?

[ollybh]

As this is an 1825 derivative which looks like it duplicates a lot of code from the 1825 implementation, it would be better if this was a subclass of G1825::Game instead of Game::Base. That would give you most of 1825’s rules without all this copied code. Though you might have to refactor the 1825 code in places to allow you to override behaviour here.

[ollybh]

@douglasfir Can you check that you haven't included more code in this pull request than you had meant to?
The PR cover note says that this game has three units and no other variants, but there is code for handling kits and regional variants. And the PR title says this is the map and companies, but there is far more than this included.

[scottredracecar]

Good advice, there are a lot of examples of inheriting games and it sure seems like the way to go for this game. 1861 inherits 1867 18USA inherits 1817 1849K2S inherits 1849 18 Los Angeles and 1852 Missouri inherit 1846 (the list goes on)

…

On Thu, Aug 8, 2024 at 10:35 AM Oliver Burnett-Hall < ***@***.***> wrote: @douglasfir <https://github.com/douglasfir> Can you check that you haven't included more code in this pull request than you had meant to? The PR cover note says that this game has three units and no other variants, but there is code for handling kits and regional variants. And the PR title says this is the map and companies, but there is far more than this included. — Reply to this email directly, view it on GitHub <#11095 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AG6CEXABGHPYDVNIYRCPDRLZQOT43AVCNFSM6AAAAABL4XALWOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENZWGMZTIMRSHE> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[douglasfir]

@douglasfir Can you check that you haven't included more code in this pull request than you had meant to? The PR cover note says that this game has three units and no other variants, but there is code for handling kits and regional variants. And the PR title says this is the map and companies, but there is far more than this included.

Sorry for the delayed response, been away from home for the past week. My original intention with this pull request was to just post my static files (map + companies + meta information). I only realized afterwards that the rakefile requires a functioning game to accept static files, so I then included my other files since they are all necessary dependencies without removing all of the 1825 vestiges.

Thanks for the advice of using a subclass; you're right, that makes much more sense. I will refactor my code accordingly, but I'm wondering if this can be releasse as an alpha quality game now to allow the designer to playtest in parallel.

[ollybh]

My original intention with this pull request was to just post my static files (map + companies + meta information). I only realized afterwards that the rakefile requires a functioning game to accept static files, so I then included my other files since they are all necessary dependencies without removing all of the 1825 vestiges.

It doesn't require a functioning game, it just needs files that can be loaded without errors. If you need to include files other than ones for the all the definitions you can just include stub files for the moment – these shouldn't cause any errors and you can add the code later on:

# frozen_string_literal: true

require_relative '../../../step/dividend'

module Engine
  module Game
    module G18TS
      module Step
        class Dividend < Engine::Step::Dividend
        end
      end
    end
  end
end

[ollybh]

Thanks for the advice of using a subclass; you're right, that makes much more sense. I will refactor my code accordingly, but I'm wondering if this can be releasse as an alpha quality game now to allow the designer to playtest in parallel.

We only have a production branch and a single production server, so code needs to be in a reasonable state before it is merged.

If you need to be able to work with the game developer to test your code before it is merged then you've got a couple of options:

1. If they've got enough computer skills they could a test instance themselves (either on their own computer or a Github codespace), clone your git repository and test the code themselves.
2. You could give them access to a development instance you are running yourself. This is what I do, I have a port forwarding rule set up on my home router to pass on requests to a computer running the server code. It might also be possible to share access to a Github codespace, I haven't done anything with these myself.