Skip to content

Add Support for Reading SpongeV3 schematics - #61

Open
DanielSchmerber wants to merge 7 commits into
PrismarineJS:masterfrom
DanielSchmerber:master
Open

DanielSchmerber wants to merge 7 commits into
PrismarineJS:masterfrom
DanielSchmerber:master

Conversation

@DanielSchmerber

Copy link
Copy Markdown

Latest FAWE now uses the SpongeV3 Schematic format when creating new Schematics.

The internal structure of them is slightly different, but the logic when parsing them stayed the same for Blocks.

This adds readonly support for SpongeV3 Schematics, and includes a test schematic.

@IceTank IceTank left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The rest looks good to me

Comment thread index.js
} catch (e) {

}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It would be good if it throws an error if it can't read the format. Right now, it could return undefined if the format is unrecognized. This is wrong for the type definition and also bad if people are wondering why they get undefined instead of a schematic or error.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

What I don't like is that all errors, even those unrelated to the format, are discarded here in favor of a generic error message that doesn't say anything about what triggered the error. There should also really be a parameter passed here to specify the schematic type, that can fallback to trying different types.

Added error message if no format can parse the schematic
@IceTank

IceTank commented Nov 7, 2024

Copy link
Copy Markdown
Contributor

@DanielSchmerber can you run npm run fix to fix the formatting error?

Fix formatting
@IceTank

IceTank commented Nov 11, 2024

Copy link
Copy Markdown
Contributor

@rom1504 I think this can be merged

@rom1504 rom1504 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.

Astra agent review — AI-generated, not manually written by the maintainer.

Rechecked b5a0ef6 and the existing discussion.

Recommend consolidating v3 block-data support into #85. It implements the explicit format selection requested here and preserves malformed-input errors, whereas this branch still catches parser failures and substitutes a generic fallback error. Those concerns already have an inline thread here, so I am not duplicating it.

Preserve any unique fixture before closing the older PR. The #85 test/spec evidence is carried from its existing review; no new schematic suite was run on this branch today.

Skills used: prismarine-protocol-data-review checked version selection and candidate-versus-release readiness; prismarine-architecture-review checked package boundaries and public compatibility; prismarine-review checked current scope and existing feedback to avoid duplicate threads.

This branch has not been deployed

No deployments
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.

4 participants