Skip to content
This repository was archived by the owner on Jun 5, 2025. It is now read-only.

Conversation

@Loufe
Copy link
Contributor

@Loufe Loufe commented Jun 13, 2024

Hi Korin, others,

I was experiencing some issues with my implementation of the post addon and while reviewing the code I gave the process function loop some thought again. I still didn't fully grok the intention with it, so I rewrote it as such. I'm not sure if this is the correct approach, I'd welcome anyone's input.

Here are my thoughts:
Regenerate the shaders (update_shaders() call) only in the following cases:

  1. When the function is called manually.
  2. When the values have been changed, provoking a change in the configuration.reload flag WHILE in the editor (experimenting) via Editor.is_editor_hint().
  3. When the values have been changed, provoking a change in the configuration.reload flag WHILE ingame for those people who have set dynamically_update to true.

I can't think of any other cases and the logic felt fuzzy with the nesting. Does this make sense to everyone else?

@Loufe
Copy link
Contributor Author

Loufe commented Jun 13, 2024

@SAED2906 any thoughts?

@KorinDev
Copy link
Owner

I think it is a good idea

@Loufe
Copy link
Contributor Author

Loufe commented Jun 14, 2024

FWIW this is working just fine for my project so far. If anyone with a traditional use of the add-on could please test it would be much appreciated so we can merge if desired.

func _process(delta):
if not configuration:
return
if Engine.is_editor_hint():
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't Line 173 be deleted? There is a parse error for line 174 because it is not indented after the if Engine.is_editor_hint(). If line 173 is kept, isn't it the same as

	if Engine.is_editor_hint() and configuration.reload and (dynamically_update or Engine.is_editor_hint()):

and will never call update_shaders() outside the editor?

Copy link
Owner

Choose a reason for hiding this comment

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

propably

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, @calvinchd , that slipped in.

@KorinDev
Copy link
Owner

should i merge, or

@Loufe
Copy link
Contributor Author

Loufe commented Jul 16, 2024

I'd appreciate a merge, we can see if any issues pop up with other users who have a different approach.

@KorinDev
Copy link
Owner

Sorry for not noticing the message, im solving some issues with cloudflare at the moment, so i will be unable to help for a day or two

@KorinDev KorinDev merged commit f4f433e into KorinDev:main Jul 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants