TeamTernate / infinite-music-discs

Tool for adding lots of custom music discs to Minecraft
82 stars 15 forks source link

Support editing internal name #115

Closed 500-internal-server-error closed 5 months ago

500-internal-server-error commented 5 months ago

Hello, as described in issue #114, I found there is currently no way to edit the internal name of the music being added, which can be quite unwieldy to type: /function <datapack_name>:give_<averylongandhardtowriteinternalid>. At the time I simply went in and manually changed the long names into the short ones which I prefer, but I figured it would be good to be able to do that from the program that generated it in the first place, rather than having to do it manually.

Additionally, while making this change, I found that the program strips out numbers, which I don't think is necessary (AFAIK the game accepts identifiers conforming to the regex ^[a-z0-9_]+$), so I have also altered the check for valid internal names to accept digits as well. I've only done 1 test case (my own datapack), but the game didn't complain and everything works as intended when I included numbers in the identifier.

bradytheinventor commented 5 months ago

Out of curiosity, what are you using the datapack for that requires you to /give yourself the custom discs? The datapack is designed for you to get the custom discs from creepers; giving yourself discs with commands was supposed to be more of a debugging feature, and the internal name is supposed to be invisible for the most part.

This is a good idea, but I think the way it's implemented here clutters the UI too much. The app is designed to be a simple tool that lets you throw a bunch of files together into a datapack, and adding lots of customization options can get confusing for some users. Especially since (I think) most people don't care what the internal name is and don't want to customize it. I have been thinking about adding an "advanced view" that's hidden by default but gives you more detailed control over the datapack you generate - this feature would fit well in something like that.

500-internal-server-error commented 5 months ago

Hi, my use case was explicitly not to have the discs obtainable in survival, instead being exclusively obtainable from a "trader" (using custom currency). I'm using it in a server with my friends, some of whom only have Bedrock Edition (Bedrock support is another feature I wanted to add, but I figured that's a problem for another day - until then I manually convert the generated resource pack).

Regarding the UI clutter, this is my first project using Qt (actually, my first project with a GUI at all - all my previous projects were headless), so I'm not very familiar with it - I simply took a look at what other thing uses a textbox that looks like the textbox I need, and changed the label to a textbox.

I suppose I could try to add in the "advanced view" idea, given sufficient guidance and time, if you're available for that.

500-internal-server-error commented 5 months ago

As a side note, could you add in a license file? As it is right now, the project is practically ARR, since there is no explicit license governing usage/contribution of the software.

bradytheinventor commented 5 months ago

Ah, I see, that's a very advanced use case! I'm impressed you're able to get the datapack to work that far outside the designed use case. Note that I intentionally don't support Bedrock because I don't have time to keep up with 2 Minecraft scripting systems - look at how long it takes me to respond to issues as it stands. So if you want to add Bedrock support, you're going to need to commit to maintain it.

Sure, I can help you with the advanced view if that's something you want to work on (and sorry if my comments came across harshly, I just wanted to explain why I think this change needs a careful implementation to avoid introducing other side effects). I want to add the advanced view myself eventually, so it's a question of whether you're willing to wait for "eventually" or want to do it yourself. If you don't want to do it then I'll just close this PR and implement all of this myself later.

The project does need a license, good catch, just created #117 to track that. I think I started looking at one point but never came to a decision.

500-internal-server-error commented 5 months ago

I'm not sure how hard getting the advanced view idea working, since as I said, I'm new to GUI programming. But I think I can give it a shot.

If you have a Discord server we could talk in, that would be an easier place to discuss things.

bradytheinventor commented 5 months ago

If you don't mind, I'd rather keep the discussion on GitHub so that others can chime in if they are interested. And anyway I don't have a Discord server set up.

I created an issue (#118) that we can use to talk about this. Let me draw up some diagrams of the UI designs I was thinking about, and then you can take a look to decide how feasible they are and get a feel for what I was planning to do. I also added some links to the Qt documentation (which is incredibly good) in case you find that helpful. And yeah if this turns out to be more than you bargained for don't feel obligated to continue! I was planning to do this myself anyway so any progress will be useful.

bradytheinventor commented 5 months ago

Closing as the discussion has moved to #118