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

Implement get_item_defs() client-side call #10455

Closed
wants to merge 2 commits into from

Conversation

appgurueu
Copy link
Contributor

Logical extension of get_item_def, another #10003 split-off.

@paramat paramat added @ Client Script API Feature ✨ PRs that add or enhance a feature labels Oct 2, 2020
@sfan5
Copy link
Member

sfan5 commented Oct 6, 2020

Do you plan to make another PR for get_node_defs .. or?

@paramat
Copy link
Contributor

paramat commented Oct 21, 2020

👎 I do not like doing this, but i disapprove because i think CSM features should not be accepted until SSCSM is implemented.
This also comes with an apology because we are not clear about the acceptance of CSM contributions and about our approach to the CSM situation, we are inconsistent. I am attempting to put this right in #10530 by getting an official decision.

@sfan5 sfan5 added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Apr 10, 2022
@rubenwardy rubenwardy added the Roadmap: Needs approval The change is not part of the current roadmap and needs to be approved by coredevs beforehand. label Apr 25, 2022
@Zughy
Copy link
Member

Zughy commented Apr 30, 2022

@appgurueu are you gonna continue with this? "Action / change needed" label, will close in 10 days if I receive no answer

@appgurueu
Copy link
Contributor Author

This is now in a mergeable state. Adding get_node_defs() would require more work (in particular exposing a list of all nodenames, which the NodeDefManager doesn't do).

@Zughy Zughy removed the Action / change needed Code still needs changes (PR) / more information requested (Issues) label May 1, 2022
@appgurueu
Copy link
Contributor Author

Closing this because CSMs seem to be a dead end anyways so far (and I will reimplement my online craftguide without requiring clients to render items). Not to mention that this should probably use a shared table to be accessed by all CSM (which would raise the question of how to protect it from writes).

@appgurueu appgurueu closed this May 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@ Client Script API Feature ✨ PRs that add or enhance a feature Low priority Roadmap: Needs approval The change is not part of the current roadmap and needs to be approved by coredevs beforehand.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants