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

allowed resources to be grown under Actors #7555

Merged
merged 2 commits into from Feb 26, 2015

Conversation

Projects
None yet
6 participants
@besuikerd
Contributor

besuikerd commented Feb 26, 2015

Fixes #7008, by modifying the Gem and Ore resource being allowed to grow under Actors. If this has some unexpected side effects please let me know

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Feb 26, 2015

Member

I expect this will also let resources grow under buildings, which we don't want. The BuildingMap may take care of that, but it would be pretty silly if the fix was this simple and hadn't been done yet. The final fix (whatever form it takes) will also need to be applied to all four mods.

Member

pchote commented Feb 26, 2015

I expect this will also let resources grow under buildings, which we don't want. The BuildingMap may take care of that, but it would be pretty silly if the fix was this simple and hadn't been done yet. The final fix (whatever form it takes) will also need to be applied to all four mods.

@besuikerd

This comment has been minimized.

Show comment
Hide comment
@besuikerd

besuikerd Feb 26, 2015

Contributor

Yes that was also my concern, I tried building several buildings directly next to a mine. Resources did not grow under those buildings. But indeed, maybe the problem should be handled in a more directed way.

Contributor

besuikerd commented Feb 26, 2015

Yes that was also my concern, I tried building several buildings directly next to a mine. Resources did not grow under those buildings. But indeed, maybe the problem should be handled in a more directed way.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Feb 26, 2015

Member

The grow resources button in the debug menu will be useful for testing this:

  1. Place a structure near a resource field.
  2. Keep pressing the grow resource button until it is completely surrounded.
  3. Sell the structure, and verify whether there is a footprint-shaped hole in the field.
Member

pchote commented Feb 26, 2015

The grow resources button in the debug menu will be useful for testing this:

  1. Place a structure near a resource field.
  2. Keep pressing the grow resource button until it is completely surrounded.
  3. Sell the structure, and verify whether there is a footprint-shaped hole in the field.
@besuikerd

This comment has been minimized.

Show comment
Hide comment
@besuikerd

besuikerd Feb 26, 2015

Contributor

Great tip, thanks!

As I can see there is an added check AllowUnderBuildings specific for buildings, so that should explain why it does not allow it under buildings

OpenRA.Mods.Common.Traits.ResourceLayer.cs

public bool AllowResourceAt(ResourceType rt, CPos cell)
        {
            if (!world.Map.Contains(cell))
                return false;

            if (!rt.Info.AllowedTerrainTypes.Contains(world.Map.GetTerrainInfo(cell).Type))
                return false;

            if (!rt.Info.AllowUnderActors && world.ActorMap.AnyUnitsAt(cell))
                return false;

            if (!rt.Info.AllowUnderBuildings && buildingInfluence.GetBuildingAt(cell) != null)
                return false;

            return true;
        }
Contributor

besuikerd commented Feb 26, 2015

Great tip, thanks!

As I can see there is an added check AllowUnderBuildings specific for buildings, so that should explain why it does not allow it under buildings

OpenRA.Mods.Common.Traits.ResourceLayer.cs

public bool AllowResourceAt(ResourceType rt, CPos cell)
        {
            if (!world.Map.Contains(cell))
                return false;

            if (!rt.Info.AllowedTerrainTypes.Contains(world.Map.GetTerrainInfo(cell).Type))
                return false;

            if (!rt.Info.AllowUnderActors && world.ActorMap.AnyUnitsAt(cell))
                return false;

            if (!rt.Info.AllowUnderBuildings && buildingInfluence.GetBuildingAt(cell) != null)
                return false;

            return true;
        }
@Mailaender

This comment has been minimized.

Show comment
Hide comment
@Mailaender

Mailaender Feb 26, 2015

Member

I can confirm that this fixes #7008 and AllowUnderBuildings also works as promised.

Member

Mailaender commented Feb 26, 2015

I can confirm that this fixes #7008 and AllowUnderBuildings also works as promised.

@besuikerd

This comment has been minimized.

Show comment
Hide comment
@besuikerd

besuikerd Feb 26, 2015

Contributor

Awesome, I'll fix it for the other mods aswell 👍

Contributor

besuikerd commented Feb 26, 2015

Awesome, I'll fix it for the other mods aswell 👍

@Phrohdoh

This comment has been minimized.

Show comment
Hide comment
@Phrohdoh

Phrohdoh Feb 26, 2015

Member

But do we want resources under actors? Specifically structures.

Member

Phrohdoh commented Feb 26, 2015

But do we want resources under actors? Specifically structures.

@penev92

This comment has been minimized.

Show comment
Hide comment
@penev92

penev92 Feb 26, 2015

Member

I don't think infantry or tanks should interfere with resources, especially tiberium (although it is illogical for ore, gems and spice). So... we really should only want tiberium to grow under tanks.
Anyway, buildings should restrict resource spreading.

Member

penev92 commented Feb 26, 2015

I don't think infantry or tanks should interfere with resources, especially tiberium (although it is illogical for ore, gems and spice). So... we really should only want tiberium to grow under tanks.
Anyway, buildings should restrict resource spreading.

@obrakmann

This comment has been minimized.

Show comment
Hide comment
@obrakmann

obrakmann Feb 26, 2015

Contributor

First of all, this works as advertised, stuff grows beneath units but not beneath buildings, so 👍

I think we should allow that for all kinds of resources as those patchy fields just look bad. Also people in the mentioned ticket are in agreement that this should be fixed, so I'm gonna merge now.

Contributor

obrakmann commented Feb 26, 2015

First of all, this works as advertised, stuff grows beneath units but not beneath buildings, so 👍

I think we should allow that for all kinds of resources as those patchy fields just look bad. Also people in the mentioned ticket are in agreement that this should be fixed, so I'm gonna merge now.

obrakmann added a commit that referenced this pull request Feb 26, 2015

Merge pull request #7555 from delftswa2014/bugfix/resourcegrowth_tran…
…sient

allowed resources to be grown under Actors

@obrakmann obrakmann merged commit a1beb8a into OpenRA:bleed Feb 26, 2015

2 checks passed

continuous-integration/appveyor AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@obrakmann

This comment has been minimized.

Show comment
Hide comment
@obrakmann

obrakmann Feb 26, 2015

Contributor

Thanks!

Changelog

Contributor

obrakmann commented Feb 26, 2015

Thanks!

Changelog

@penev92

This comment has been minimized.

Show comment
Hide comment
@penev92

penev92 Feb 26, 2015

Member

Yeah, I was saying that it was illogical, not that we shouldn't have it :)
The C&C games have enough weird things like this :D

Member

penev92 commented Feb 26, 2015

Yeah, I was saying that it was illogical, not that we shouldn't have it :)
The C&C games have enough weird things like this :D

@jabbink jabbink deleted the delftswa2014:bugfix/resourcegrowth_transient branch Apr 9, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment