Remove new instance of EngineModule for each UI settings change

[Vandest1]

I don't why there was basically this settings change check but today it isn't necessary anymore. It make a new EngineModule each time you change values by UI toolbars and make UI toolbars unusable. So it need to remove it.

[linuxgurugamer]

Ummm, I just looked at the code, and it only creates a new engine module if it hasn't been created already, see this: https://github.com/linuxgurugamer/EngineLightRelit/blob/b112d3c1cf64030bfc3e569f2a34fc1389128473/EngineLightRelit/EngineLightEffect.cs#L345

The rest of that method is setting up things which might have changed due to the player changing something in the stock settings page.

So unless you can demonstrate that it really is creating a new engine module, this will not be merged

[Vandest1]

You right, I wasn't very explicit in my first PR comment.

Ummm, I just looked at the code, and it only creates a new engine module if it hasn't been created already, see this:

https://github.com/linuxgurugamer/EngineLightRelit/blob/b112d3c1cf64030bfc3e569f2a34fc1389128473/EngineLightRelit/EngineLightEffect.cs#L345

Effectively, it seems strange. But may be it's cause by the fact we just call initEngineLights method from FixedUpdate method, without specify about which engineModule is. So, in initEngineLights this engineModule is null, and it create a new one. And like initEngineLights isn't a method of engineModule class,, to know which engineModule object it is, may be, pass this engineModule like an argument to initEngineLights could work.

But, the ConfigChanged check seems useless, both in stock settings page and in EnginLight toolbar settings. If you want to see it, you can try it in game (I've send it to you in pm on ksp forum, but I also put it on the end of this comment). You can also test current version in game, and try to change settings by EnginLight toolbar window and see what happen. For me it create a new light source each time I moving settings bars, which makes it unusable.

I'm a junior developer and you certainly are a developer much more skilled than me, so if you found a better solution I'm all ears and I would be delighted to learn more. But this bug really exist.

EngineLightRelit.zip

[linuxgurugamer]

Hmmm. The problem isn't the engine module, look further down, I think the problem is here: https://github.com/linuxgurugamer/EngineLightRelit/blob/b112d3c1cf64030bfc3e569f2a34fc1389128473/EngineLightRelit/EngineLightEffect.cs#L387-L389

I think I know what to do, but as an exercise, see if you can figure this out :-)

[Vandest1]

Thank you for exercise. :-p Effectively, it could result from this part of code.

This could resolve the issue: if (engineLightObject == null) engineLightObject = new GameObject();

So it wasn't some new instances of engineModule that occur this issue but some new instances of engineLightObject. I should have thought about it by writing "For me it create a new light source each time I moving settings bars".

[linuxgurugamer]

Try this, please: https://www.dropbox.com/s/872znnv462ppdel/EngineLightRelit-1.6.3.2.zip?dl=0

[Vandest1]

It working good, no more bug with toolbar window settings. So, it was engineLightObject ?

[linuxgurugamer]

There were several things like that, In addition to that, there were four emissives being created each time. Look the diff when I do the release later today to see what changed

[linuxgurugamer]

I'll close this PR when I do the release so you get notified

[Vandest1]

I didn't notice about emissives, I didn't have my eyes on it. Also, I notice that you included AddComponent in engineLightObject existence check. I was thinking that there could only be one component per type and that overwrite the old one, but after reading Unity doc I saw it's possible to get it several. So right now, EngineLight should work like a charm, thank you very much.