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

SimplePrep #366

Conversation

StandingPadAnimations
Copy link
Collaborator

@StandingPadAnimations StandingPadAnimations commented Jan 27, 2023

The recent Kaion release introduced a new feature called SimplePrep. While still in development, I've opened this PR to track development.

Right now the only thing that needs to be implemented is allowing users to pick and choose the parts of the UI they want to remove, but what do y'all think?

@StandingPadAnimations
Copy link
Collaborator Author

Why do I keep making PRs to the wrong repo

@StandingPadAnimations StandingPadAnimations deleted the simple-prep branch January 27, 2023 00:40
@StandingPadAnimations StandingPadAnimations restored the simple-prep branch January 27, 2023 00:41
@StandingPadAnimations StandingPadAnimations changed the base branch from master to dev January 28, 2023 00:09
@StandingPadAnimations
Copy link
Collaborator Author

Reopened since this feature is now part of the latest Kaion release

@StandingPadAnimations
Copy link
Collaborator Author

So there's definitely a bug when SimplePrep is disabled after being enabled. For some reason, SimplePrep is unable to reregister classes. Why this is the case, I have no idea (StandingPadAnimations#5 also doesn't help when trying to read errors).

The bug seems to exist here:

def simple_prep_func(self, context):
	global SIMPLE_UI
	if self.simple_prep:
		unsimple_list = get_unsimple_list(self.simple_prep_remove_mcprep_add, 
										self.simple_prep_remove_skins, 
										self.simple_prep_remove_world_tools, 
										self.simple_prep_remove_spawner)
		
		# Remove classes that are already in SIMPLE_UI
		unsimple_list = [cls for cls in unsimple_list if cls not in SIMPLE_UI]
		for cls in unsimple_list:
			bpy.utils.unregister_class(cls)

		# Reregister classes
		simple_class_reregister(self.simple_prep_remove_mcprep_add, 
							self.simple_prep_remove_skins, 
							self.simple_prep_remove_world_tools, 
							self.simple_prep_remove_spawner)
		

	else:
		# Check to make sure we have classes in this list as to avoid unregistering already unregistered classes
		if len(SIMPLE_UI) > 0:
			for cls in SIMPLE_UI:
				bpy.utils.register_class(cls)
		SIMPLE_UI = []
		

SIMPLE_UI is a global list that holds all unregistered classes. When SimplePrep reregisters a class (with SimplePrep enabled), it calls the following function:

def simple_class_reregister(mcprep_add, skins, world_tools, spawner):
	global SIMPLE_UI
	if not mcprep_add:
		if MCPREP_MT_3dview_add in SIMPLE_UI:
			bpy.utils.register_class(MCPREP_MT_3dview_add)
			SIMPLE_UI.remove(MCPREP_MT_3dview_add)

	if not skins:
		if MCPREP_PT_skins in SIMPLE_UI:
			bpy.utils.register_class(MCPREP_PT_skins)
			SIMPLE_UI.remove(MCPREP_PT_skins)

	if not world_tools:
		if MCPREP_PT_world_tools in SIMPLE_UI:
			bpy.utils.register_class(MCPREP_PT_world_tools)
			SIMPLE_UI.remove(MCPREP_PT_world_tools)
	
	if not spawner:
		spawn_classes = (
			MCPREP_PT_spawn, 
			MCPREP_PT_mob_spawner, 
			MCPREP_PT_item_spawner,
			MCPREP_PT_model_spawner,
			MCPREP_PT_entity_spawner,
			MCPREP_PT_effects_spawner,
			MCPREP_PT_meshswap_spawner,
		)
		# Only get the classes that are in the SIMPLE_UI variable
		rereg_classes = [cls for cls in spawn_classes if cls in SIMPLE_UI]
		for cls in rereg_classes:
			bpy.utils.register_class(cls)
			SIMPLE_UI.remove(cls)

Here, we see what options are False (signifying the user wants to keep them) and we register them again and remove them SIMPLE_UI. I have no idea where the bug occurs exactly, but I know it has to be in there.

@StandingPadAnimations
Copy link
Collaborator Author

2 major bugs fixed, this now works

Before this can be merged #369 has to be merged because this contains commits from that. Alternatively, we could close that and merge this in

Copy link
Member

@TheDuckCow TheDuckCow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once again just leaving comment as I need to better understand the net before/after on this change.

Can you also share the primary motivation for this change? Is there a reason to not just gate this functionality by varying what is shown/hidden using user preferences options? (and then checking these options during draw calls)?

MCprep_addon/mcprep_ui.py Outdated Show resolved Hide resolved
MCprep_addon/mcprep_ui.py Outdated Show resolved Hide resolved
classes = classes + [cls for cls in spawn_classes]
return classes

def simple_class_reregister(mcprep_add, skins, world_tools, spawner):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a screenshot of what this UI looks like and high level explain the differences made by these simple variants? I see the release here, but not sure what it looks like in the broader context

At a high level, I'm weary of registering and unregistering classes midway through script execution, it's a bit abnormal. I'm much more okay with dynamically deciding to draw different already-registered UI elements based on preferences options. The memory overhead is negligent to have both and conditionally draw.

@StandingPadAnimations
Copy link
Collaborator Author

StandingPadAnimations commented Feb 6, 2023

Here are some example screenshots. The user can choose what to remove (as of the February release):
image
image

At a high level, I'm weary of registering and unregistering classes midway through script execution, it's a bit abnormal. I'm much more okay with dynamically deciding to draw different already-registered UI elements based on preferences options. The memory overhead is negligent to have both and conditionally draw.

Same here, though the reason for unregistering is mainly because Blender doesn't have a proper way to "undraw" a part of the UI as far as I know, which is why we unregister the classes. I initially added if statements to remove the text and whatnot, but that left empty panels which were annoying to deal with

@TheDuckCow
Copy link
Member

Gotcha, and fair reason. I think that can actually be accomplished using a context poll. See this sample code pulled from the bpy template, I added the poll function:

In the end though, still not totally convinced about this. Panels can easily be collapsed. But....if you, as a longtime user and developer specifically want this, I suppose there's little harm to adding preferences property tick boxes (default to show panels, of course). Probably a display section where you can enable/disable panels this way. Then the corresponding panels would just check against that prefs property to return poll true or false. Sorry if this undos a lot of your work, but I think would be much cleaner.

import bpy

class HelloWorldPanel(bpy.types.Panel):
    """Creates a Panel in the Object properties window"""
    bl_label = "Hello World Panel"
    bl_idname = "OBJECT_PT_hello"
    bl_space_type = 'PROPERTIES'
    bl_region_type = 'WINDOW'
    bl_context = "object"
    
    @classmethod
    def poll(self, context):
        return False

    def draw(self, context):
        layout = self.layout

        obj = context.object

        row = layout.row()
        row.label(text="Hello world!", icon='WORLD_DATA')

        row = layout.row()
        row.label(text="Active object is: " + obj.name)
        row = layout.row()
        row.prop(obj, "name")

        row = layout.row()
        row.operator("mesh.primitive_cube_add")


def register():
    bpy.utils.register_class(HelloWorldPanel)


def unregister():
    bpy.utils.unregister_class(HelloWorldPanel)


if __name__ == "__main__":
    register()

@StandingPadAnimations
Copy link
Collaborator Author

StandingPadAnimations commented Feb 7, 2023

Panels can easily be collapsed.

That's a fair point, it's a more niche feature. I think it'll make sense to close this

@StandingPadAnimations
Copy link
Collaborator Author

By the way, in case you're worried, I'm actually fine with keeping this as a niche feature in a fork. I think it makes better sense for niche features to not affect maintainbility of a codebase (especially for more complex features)

@TheDuckCow
Copy link
Member

Sure thing, no complaints about that! Appreciate it @StandingPadAnimations

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.

None yet

2 participants