-
-
Notifications
You must be signed in to change notification settings - Fork 54
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
Formalize most public API #253
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lots of good stuff in here, just a few things that caught my eye
if (level == null) { | ||
if (state == null) { | ||
state = Blocks.AIR.defaultBlockState(); | ||
} | ||
FabricOriginBlockAndTintGetter originLevel = objects.level; | ||
originLevel.originBlockState(state); | ||
level = originLevel; | ||
} else if (state == null) { | ||
state = level.getBlockState(BlockPos.ZERO); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like complex behavior to hide in the impl. Somewhat related, perhaps there should be explicit methods for defaultLevel and defaultBlockState in the common builder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All builder methods for all three types of builders are optional and thus have default values. Would it be better to add default*()
methods for all properties or accept nullable parameters, where null means default?
common/src/lib/java/dev/engine_room/flywheel/lib/model/baked/PartialModel.java
Outdated
Show resolved
Hide resolved
- Add Engine#onLightUpdate; remove LightUpdateHolder and backend/ClientChunkCacheMixin - Add Effect#level - Add VisualizationHelper#queueAdd and #queueRemove for Effects - Fix PartialModel not assigning bakedModel field when populating on init - Fix PartialModel.ALL using weak keys instead of weak values - Make Simple*Visualizer and corresponding inner Builder classes final - Restore FlatLit#light overload that accepts block and sky light values separately - Add AbstractBlockEntityVisual#relight overloads that accept Iterator and Iterable - Reorganize classes in impl.vizualization
- Move TaskExecutor#sync* methods to TaskExecutorImpl - Move Flag and RaisePlan to impl - Remove TaskExecutor#scheduleForMainThread and #isMainThread methods - Remove SyncedPlan - Add Engine#setupRender - Remove TaskExecutor parameters from Engine#render* methods - Convert Engine$CrumblingBlock into an interface - Unmark RenderContext as NonExtendable to allow fulfilling the purpose described in the doc of VisualizationManager#renderDispatcher
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice changes ❤️
- Lazily initialize MaterialShaderIndices - Rename MaterialShaders#*Shader to #*Source - Move BackendImplemented to api.backend package
I will merge the PR now as there are a lot of changes and waiting for more minor changes is not worth the merge conflicts with other mainline changes that this will cause.
|
This pull request performs many, mostly minor, changes across most public API to make it nicer to use and more stable in general.