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

Blocks implementations #314

Closed
wants to merge 9 commits into from
Closed

Blocks implementations #314

wants to merge 9 commits into from

Conversation

Dhaiven
Copy link
Contributor

@Dhaiven Dhaiven commented Jul 21, 2024

This pr is not finished.
I have just a question, why there are BlockTypeDefaultInitializer and BlockTypeInitializer ?

@Dhaiven Dhaiven marked this pull request as draft July 21, 2024 21:40
@Dhaiven
Copy link
Contributor Author

Dhaiven commented Jul 21, 2024

Another question. Why not add the ability to add multiple setBlockBaseComponentSupplier instead of just one.
Indeed, I try to add the leaves but it is necessary to have a "custom class" because the leaves have several properties (unlike the log which has only one).
However, if we can add multiple classes, there is no need to have a 'custom class' because each class handles on the property.
This would remove a lot of code repetition.

@IWareQ
Copy link
Member

IWareQ commented Jul 22, 2024

This pr is not finished.

I have just a question, why there are BlockTypeDefaultInitializer and BlockTypeInitializer ?

Hello, BlockTypeDefaultInitializer is generated automatically so that blocks can be used. BlockTypeInitializer already implements the full functionality of these blocks, and is loaded before the default initializer.

@Dhaiven
Copy link
Contributor Author

Dhaiven commented Jul 22, 2024

ok

@smartcmd
Copy link
Member

I think you may have misunderstood the base component. The base component depends on block instead of property

@Dhaiven
Copy link
Contributor Author

Dhaiven commented Jul 22, 2024

Je pense que vous avez peut-être mal compris la composante de base. Le composant de base dépend du bloc plutôt que de la propriété

Indeed, I misunderstood

@smartcmd
Copy link
Member

Any way, thanks for the contributions! (Even when I haven't write docs for the component system)

@Dhaiven
Copy link
Contributor Author

Dhaiven commented Jul 22, 2024

I think you may have misunderstood the base component. The base component depends on block instead of property

I'll come back to that. The "problem" with the base component depending on the block is that once all the drops are added, each block will have almost that class and won't be able to share like for example the leaves. For me (and this is why I misunderstood) the word base refers to a property, a class being implemented by others as with faces for example. I think it would be better and more intuitive to use base for properties rather than the components.
I await your opinion.

@smartcmd
Copy link
Member

I think you may have misunderstood the base component. The base component depends on block instead of property

I'll come back to that. The "problem" with the base component depending on the block is that once all the drops are added, each block will have almost that class and won't be able to share like for example the leaves. For me (and this is why I misunderstood) the word base refers to a property, a class being implemented by others as with faces for example. I think it would be better and more intuitive to use base for properties rather than the components. I await your opinion.

I fully understand and agree what you said. This is indeed a problem and I will take your suggestion into consideration.

@Dhaiven
Copy link
Contributor Author

Dhaiven commented Jul 22, 2024

if you want, i can do this change in this pr

@smartcmd
Copy link
Member

I have an idea, we can create a processor for each block property. When a block is being placed, we foreach the block's properties and then find processor for each property, let's the processor decide the block property's value based on the placement information

@smartcmd
Copy link
Member

Can you join our discord server to have more detailed discussion? We haven't decide the solution

@Dhaiven
Copy link
Contributor Author

Dhaiven commented Aug 14, 2024

Move to #341

@Dhaiven Dhaiven closed this Aug 14, 2024
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.

3 participants