Skip to content

Fix FAWE compat, keep fast blockstate palette - #199

Merged
Nrleryxx merged 1 commit into
GaleMC:mainfrom
royvortex:main
Sep 13, 2026
Merged

Nrleryxx merged 1 commit into
GaleMC:mainfrom
royvortex:main

Conversation

@royvortex

Copy link
Copy Markdown
Contributor

Motivation
FAWE crashes on Gale with NoSuchFieldError: Block does not have member field 'IdMapper BLOCK_STATE_REGISTRY'. Gale's blockstate-palette optimization changed the public field's type from IdMapper to the custom BlockStateIdMapper, so any plugin compiled against vanilla/Paper (FAWE's PaperweightFaweAdapter.adaptToChar) fails to link.
Changes

  • BlockStateIdMapper now extends IdMapper instead of implements IdMap, keeping the field type plugin-compatible. Writes mirror to both vanilla storage and the array; getId() uses the intrusive index and byId() uses the array, so both hot paths stay fast.
  • Block.java.patch: field declared as IdMapper again.
  • Blocks.java.patch: trimToSize() via cast.
  • build-data/gale.at: un-final vanilla IdMapper.byId so it can be overridden.
    Context
    Was AI used to write this pull request: Yes

@Nrleryxx Nrleryxx added the type: fix bug fix etc related PRs label Sep 13, 2026

@Nrleryxx Nrleryxx left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  1. Block.java.patch currently has two plus lines adding the same static final field name
    after removing the old IdMapper line both of these would end up in the patched file
    producing duplicate field compile error
    also the first added line declares type as BlockStateIdMapper which would break the FAWE compat goal anyway
    fix keep only the plus line that declares IdMapper BlockState BLOCK_STATE_REGISTRY completely delete the other plus line
    you should end up with exactly one minus and one plus for that field

  2. Blocks.java.patch has two separate plus lines adding trimToSize calls and no deletions
    both end up in final code causing redundant calls
    worse the first line calls Block BLOCK_STATE_REGISTRY trimToSize directly without cast
    after the field type changes to IdMapper this becomes compile error because vanilla IdMapper class has no trimToSize method
    fix keep only the cast version and fully delete the uncast direct call line

@Nrleryxx Nrleryxx left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

after changing class from implements IdMap to extends IdMapper you now rely on inherited methods for byId iterator size and trimToSize
those inherited implementations use the slow vanilla ArrayList internal storage from IdMapper not our fast idToT array
this completely negates the intrusive array backed performance optimization back to slow vanilla speeds
fix keep and explicitly override all four of these in BlockStateIdMapper making sure they read from idToT and our own size field not the parent storage
byId with Nullable return the fast array bounds checked lookup
iterator return the existing anonymous inner class walking idToT directly
size return this size
trimToSize Arrays copyOf idToT trimmed to this size
the access transformer for IdMapper byId final removal only helps if you actually override the method otherwise nobody benefits

@Nrleryxx Nrleryxx left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the new getId method taking state parameter is missing the Override annotation above it
the annotation is still attached to the old thing variant which is being removed
not a compile error but please add the annotation to the new state based variant

@royvortex

Copy link
Copy Markdown
Contributor Author

Thanks for reviewing! I checked each point against this commit (cc45af0, Files tab):

  1. Block.java.patch has exactly one minus/one plus for the field (lines 7–8), declaring IdMapper — no duplicate field, no BlockStateIdMapper-typed line.
  2. Blocks.java.patch has exactly one plus line (line 7), the cast version — no uncast call.
  3. byId (line 68), iterator (line 73), size (line 95), and trimToSize (line 99) are all overridden on the array in BlockStateIdMapper.java — nothing falls back to vanilla storage on hot paths.
  4. @OverRide is present on the state-based getId (lines 62–63).

@Nrleryxx

Copy link
Copy Markdown
Member

shit my bad github files tab served me a 10 hour stale cache of the diff
pulled the raw file links from the commit instead and yeah everythings 100% good
all 4 things u said are actually there no issues anymore
my requested changes were completely based on old cached content sorry
approve from me

Thanks for the pr!
Merged!

@Nrleryxx
Nrleryxx merged commit 7e97ae8 into GaleMC:main Sep 13, 2026
1 of 2 checks passed
@Dreeam-qwq

Copy link
Copy Markdown
Collaborator

Didn't check FAWE's code, but if FAWE just accesses the BLOCK_STATE_REGISTRY and calls its interface methods, then we can just extends IdMapper and override the methods we need. I think we don't need to call super and write mirror data back to IdMapper.

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

Labels

type: fix bug fix etc related PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants