blockbench-plugins icon indicating copy to clipboard operation
blockbench-plugins copied to clipboard

Update Mod Utils

Open Ocraftyone opened this issue 2 years ago • 15 comments

I identified some improvements that I wanted to make to this plugin. I was going to create another plugin, but I figured I would see if I could just contribute to the original one rather than make a spinoff. While writing it, I instinctively used a formatter which touched almost every piece of code. I left it this way out of laziness, but if this is unwanted, I would be happy to submit one without the formatting.

Added more settings to the dialog -Parchment is now a mappings option -Option to disable the group based logic on group names -Option to disable requirement of the "VoxelShapes" group -Option for remembering the settings -Pressing shift while pressing the voxel export button now reopens the dialog, even if settings are set to be remembered

Version number updated Added myself as an author

Ocraftyone avatar Sep 26 '22 06:09 Ocraftyone

-Option to disable requirement of the "VoxelShapes" group

This is something I would massively advise against. Most people do not consider how massive the performance impact of an unoptimized Voxel Shape can be. That's exactly why this group requirement was added, so people spent a moment thinking about it.

I also don't agree with disabling the group name logic part, it's ment for optimization. People should not be encouraged to just disable all optimization and ignore it.

Everything else is nice though.

JTK222 avatar Oct 01 '22 22:10 JTK222

Displaying the resulting VoxelShape code in a dialog is a very welcome addition 😄

Elenterius avatar Oct 02 '22 01:10 Elenterius

-Option to disable requirement of the "VoxelShapes" group

This is something I would massively advise against. Most people do not consider how massive the performance impact of an unoptimized Voxel Shape can be. That's exactly why this group requirement was added, so people spent a moment thinking about it.

This I can understand. I just have seen to many tutorials where the whole model is just put in the Voxel group, so I thought this could be convenient for those who are looking to do that.

I also don't agree with disabling the group name logic part, it's ment for optimization. People should not be encouraged to just disable all optimization and ignore it.

Yes. This feature is awesome, but it becomes a pain for those who use groups for organization or those who want have everything use the OR function

I would be happy to hide this behind an advanced settings section with documentation and warnings, but I think that these should be implemented in some capacity for advanced users.

Ocraftyone avatar Oct 02 '22 01:10 Ocraftyone

Displaying the resulting VoxelShape code in a dialog is a very welcome addition 😄

Yup! I based it on how the Code View plugin did it

Ocraftyone avatar Oct 02 '22 01:10 Ocraftyone

-Option to disable requirement of the "VoxelShapes" group This is something I would massively advise against. Most people do not consider how massive the performance impact of an unoptimized Voxel Shape can be. That's exactly why this group requirement was added, so people spent a moment thinking about it.

This I can understand. I just have seen to many tutorials where the whole model is just put in the Voxel group, so I thought this could be convenient for those who are looking to do that.

This shouldn't be convenient because 99% of the time it's something you shouldn't do. Yes, people are lazy, but being lazy and doing it "wrong" shouldn't be the easy way of doing it. Blockbench removed the noise tool for the same reason, because it was the easy and lazy way of "texturing" but produced bad results.

I also don't agree with disabling the group name logic part, it's ment for optimization. People should not be encouraged to just disable all optimization and ignore it.

Yes. This feature is awesome, but it becomes a pain for those who use groups for organization or those who want have everything use the OR function

I would be happy to hide this behind an advanced settings section with documentation and warnings, but I think that these should be implemented in some capacity for advanced users.

I can see that this might cause some issues, however none of the possible group names, except for FIRST and SECOND should really run a risk of overlapping. Maybe adding adding a prefix of some kind like changing them to $FIRST and $SECOND, could be a good solution to this problem. However again, this is only a problem when lazy people just drag their whole model into this group, which should be discouraged.

JTK222 avatar Oct 02 '22 08:10 JTK222

I understand that first point and when messing around with voxel shapes, I have seen the performance impact. Obviously not good. The reason I had for this (other than thinking it was redundant) was the visuals of the outliner. I'm a neat freak when it comes to coding and modeling and I like to stay organized. The group just looks out of place and I just didn't like it. More of a personal preference rather than a functional one.

What I had in mind while removing the group based logic was making it so that you could group your shapes however you like. Personally, I will organize my model into multiple parts using groups. Then when you export the voxel shape, it adds these group names as bogus BooleanOps. I completely agree that adding a prefix should solve the problem because then you can call your group whatever you want without it adding the operation. Another option would be to only act on the group names that are valid boolean functions.

When taking on this project, I did not see these problems. While now that you have pointed, I understand the impact of these. I think one step could be putting these options in an advanced settings menu. This would provide the benefit of having the modularity while also having the user think about the actions they are taking. This has been used in other applications, like settings for routers that normal people shouldn't change (dns settings, MAC address settings). Just put a scary message about how using these settings can cause major performance issues and to make sure you know what you are doing before using them. LMK what you think about that idea.

Ocraftyone avatar Oct 02 '22 08:10 Ocraftyone

When taking on this project, I did not see these problems. While now that you have pointed, I understand the impact of > these. I think one step could be putting these options in an advanced settings menu. This would provide the benefit of having the modularity while also having the user think about the actions they are taking. This has been used in other applications, like settings for routers that normal people shouldn't change (dns settings, MAC address settings). Just put a scary message about how using these settings can cause major performance issues and to make sure you know what you are doing before using them. LMK what you think about that idea.

Generally you are right, however in Blockbenches case it's problematic. People will blindly follow tutorials without even spending a thought on it. If a tutorial says "just ignore this" people will ignore it. Also, I thought that it already does only consider valid group names, if not that was a mistake.

Tbh, do what you think is best. I provided you my opinions and worries, I am not using the plugin personally, so do what you think is best for people.

I planned to do a V2 of Mod Utils at one point, where Voxel Shapes are a 3rd model object, additional to groups & cubes. With the function to generate them for the model. Not sure when I'll get around doing that though, I am still not sure whether I understand well enough how BB works to do such stuff.

JTK222 avatar Oct 02 '22 13:10 JTK222

Generally you are right, however in Blockbenches case it's problematic. People will blindly follow tutorials without even spending a thought on it. If a tutorial says "just ignore this" people will ignore it.

Oh boy, you bet they do. This is already done with tutorials that just say "throw everything in this group". I have already seen that. At least this way, there could be an explanation on why you shouldn't do it. I'll make an implementation of what I am envisioning and see if you think it is adequate. In the end, this is your project and I want to make sure that you approve of it.

Also, I thought that it already does only consider valid group names, if not that was a mistake.

Yup. I'll look in to either implementing a system that checks for a valid group name or just require a symbol before the name for it to be considered. Heck, maybe even a combination of the both.

I planned to do a V2 of Mod Utils at one point, where Voxel Shapes are a 3rd model object, additional to groups & cubes. With the function to generate them for the model.

I would hope this isn't too hard. You may be able to implement something similar or make it a subclass to a group. Either way, this would solve both of these issues with the right implementation.

Ocraftyone avatar Oct 03 '22 09:10 Ocraftyone

I had one other question. When designing this, were you envisioning people using their actual model to generate the voxelshapes or were you thinking more down the line of a separate model that represented the voxelshape?

Ocraftyone avatar Oct 03 '22 09:10 Ocraftyone

I hoped that people would go down the line of modelling the Voxel Shape seperately, matching their model, but being more optimized. E.g. a cauldron is usually modelled using 5 cubes, the voxel shape for it, can be 2 cubes. In many other situations, people have tiny details, that don't make sense to be also included into the voxel shape.

JTK222 avatar Oct 03 '22 15:10 JTK222

All right. I think that I have made some good changes here. @JTK222 If you could look over these when you get a chance and see if you like the implementation. I have implemented the change that requires the "$" before any boolean function for it to be recognized, added an advanced menu that allows you to toggle the VoxelShapes group, and updated the help menu

Ocraftyone avatar Oct 09 '22 06:10 Ocraftyone

@JannisX11 I am personally happy with the current implementation. Do you see any issues with this? I would still love to hear what @JTK222 thoughts are on it but they seem to have been unresponsive for a couple of weeks.

Ocraftyone avatar Oct 24 '22 11:10 Ocraftyone

I have added maintainer edits to this PR too

Ocraftyone avatar Oct 24 '22 11:10 Ocraftyone

Sorry forgot to answer last time. Like I said, I am not a fan of providing easy workflows that have performance drawbacks for the end user, users shouldn't be lazy with such stuff, but should put the effort & thought into doing it properly. (Mods do already have a bad enough reputation, in regards to performance)

But I am not the one to decide this, especially as I am not using this plugin myself. The code looks fine from what I've seen, therefore you've got my approval.

JTK222 avatar Oct 24 '22 12:10 JTK222

For fu*** sake, how often will I again forget to send my answer >.<

Sorry, for the late answer, have the bad habbit of checking such stuff at work and always forget to either answer, or click the stupid comment button. So in regards to this PR:

Like I said before, I am against creating workflows for doing things the "lazy" way. Especially like in this case with VoxelShapes at the cost of Performance. To little people do already put any thought into it and mods as they are do already have a bad enough reputation for their performance.

However, I am not the one to decide the future of this, especially as I do not use this plugin but a custom version of it. But most other stuff are good changes & the code looks fine to me, except that I'd maybe bump the version to 1.8, the changes are substantial enough for that.

EDIT: AHHH! GH didn't show my last message as sent 😭

JTK222 avatar Oct 25 '22 06:10 JTK222

Not sure what to do with this PR.

JannisX11 avatar Nov 25 '22 22:11 JannisX11