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

New block families by pollend - merge to develop post-GCI #3149

Closed
wants to merge 33 commits into from

Conversation

Cervator
Copy link
Member

@Cervator Cervator commented Nov 27, 2017

Update: Newer rebased PR available: #3343

Contains

New block family system by @pollend - see this Gist for some documentation and philosophy on the change as well as original PR #3085

How to test

Will need to experiment some with an existing block family or make a new one. Some directions in the Gist but more details for here would be good. Probably Rails would be the ultimate test case.

Outstanding before merging

This breaks backwards compatibility and is part of the v2.0.0 engine release. Some modules are impacted and should be updated with PRs that make the associated changes required by this PR. A task chain for GCI is based on this PR's branch and should be allowed to finish with additional improvements PRed to the branch before this is merged.

  • MarcinScIncubator
  • ... more modules
  • Await end of GCI for final feedback and additional changes (should be merged into the newBlockFamilies branch

Additional comments in this PR for feedback/improvements would also be appreciated :-)

@Cervator Cervator added API Topic: Architecture Requests, Issues and Changes related to software architecture, programming patterns, etc. Blocker Issue reporting or PR addressing a critical problem that blocks other efforts Category: Gameplay Content Requests, Issues and Changes targeting gameplay mechanics and content Status: Needs Investigation Requires to be debugged or checked for feasibility, etc. labels Nov 27, 2017
@Cervator Cervator added this to the v2.0.0 milestone Nov 27, 2017
@GooeyHub
Copy link
Member

Hooray Jenkins reported success with all tests good!

@@ -69,6 +71,17 @@ public void largeBlockUpdateFinished(LargeBlockUpdateFinished event, EntityRef e
}
}

@ReceiveEvent(components = {BlockItemComponent.class})
public void onPlaceBlock(OnBlockItemPlaced event, EntityRef entity) {
Copy link
Member

@pollend pollend Nov 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so this was for the placement problems where a block is placed, but the neighboring blocks don't update. This would come with cables is signalling where if you placed a block the neighboring cables won't update to reflect the new connection.

I think there might be a problem with BlockItemComponent.class because I believe this would never call if the block does not have an item tied to it. This was once of the gci submits and I had to remove the BlockItemComponent for this to work correctly. https://gist.github.com/pollend/148d45ae964337358887b2ab1d4cfeaf

So would I do this instead:
@ReceiveEvent()
public void onPlaceBlock(OnBlockItemPlaced event, EntityRef entity) {
...

@@ -29,13 +32,23 @@
private BlockUri uri;
private Set<String> categories = Sets.newHashSet();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not make much sense in the scope of families. these are already provided by the block itself.

@GooeyHub
Copy link
Member

GooeyHub commented Dec 1, 2017

Hooray Jenkins reported success with all tests good!

@Steampunkery
Copy link
Contributor

When you merge this PR, also merge this one into fences so it uses the new block families.

@PS-Soundwave
Copy link
Member

Definitely a good cleanup of the previous system. Me, @pollend, and @Cervator have discussed these changes as well as future considerations extensively, and I've summarized what I got from it in a gist below. I of course cannot speak for them, but I attempted to accurately portray where we're at right now.

https://gist.github.com/PS-Soundwave/799fd55d34b06b092e7277654540299a

@anuar2k
Copy link
Contributor

anuar2k commented Jan 13, 2018

Terasology-Archived/MarcinScIncubator#2 took a try on the first entry on the list, @Cervator :P

Minor fixes to new block families' docs (merge to newBlockFamilies branch)
@GooeyHub
Copy link
Member

Hooray Jenkins reported success with all tests good!

@pollend
Copy link
Member

pollend commented Jan 31, 2018

there are some secondary commits that will also need to be addressed:

#2907
#3180

@GooeyHub
Copy link
Member

Hooray Jenkins reported success with all tests good!

@GooeyHub
Copy link
Member

Hooray Jenkins reported success with all tests good!

@Cervator
Copy link
Member Author

Cervator commented Apr 27, 2018

Edit: See #3343 for newer comments - that PR is rebased and easier to test with modules

Been working on preparing this better as the base branch is pretty old at this point, making it hard to test some modules.

Merged in both #3221 and #3180 including conflict fixes (to the newBlockFamilies branch, not develop). This means existing module repo PRs that adjust for this PR may need slight additional tweaks on top. I'm looking at that next.

Some todos remain

I might take a stab at rebasing this, but in a new branch + PR just in case. - #3343

Related/updated module PRs

PRs exist but need more work

Modules that still need a PR to fix related code

  • TutorialAssetSystem (and probably its wiki as well)
  • GrowingFlora
  • ItemPipes
  • LightAndShadow
  • Minesweeper - seemingly just needs a standard family update
  • ModularComputers - single method no longer matches a signature
  • Rails
  • Sample - special since it has several old families from GCI including PRs to update
  • StructuralTemplates

Unknown

Can't compile due to broken dependencies. Could be fine after dependencies fixed

  • Lost - probably OK, fails from GrowingFlora not building
  • NeoTTA - probably OK, fails from GrowingFlora not building
  • WoodAndStone - probably OK, fails from GrowingFlora not building

Also: potentially any module that uses block families the old school way in prefabs rather than define their own unique block families thus causing compilation errors (I haven't looked for those)

@Cervator
Copy link
Member Author

Block families

@pollend
Copy link
Member

pollend commented May 10, 2018

There is already a pr for this #3343.

Cervator added a commit that referenced this pull request May 26, 2018
@Cervator
Copy link
Member Author

Closing as merged by #3343

@Cervator Cervator closed this May 26, 2018
@Cervator Cervator deleted the newBlockFamilies branch May 26, 2018 22:36
@skaldarnar skaldarnar removed the Api label May 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocker Issue reporting or PR addressing a critical problem that blocks other efforts Category: Gameplay Content Requests, Issues and Changes targeting gameplay mechanics and content Status: Needs Investigation Requires to be debugged or checked for feasibility, etc. Topic: Architecture Requests, Issues and Changes related to software architecture, programming patterns, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.