search

OpenCollarTeam / OpenCollar

Other
123 stars 128 forks source link

Switch to Firestorm preprocessor? #39

Closed nirea closed 6 years ago

nirea commented 6 years ago

This is an issue that I've gone back and forth on. I used to be strongly opposed to using the Firestorm preprocessor because I wanted people to be able to just open up their collars and see the same code that they'd see on Github. Accessibility of the code for new users is important to me.

On the other hand, in going through the code tonight, I'm seeing TONS of duplication. I already knew that most scripts have the same constants at the top, but there are also text strings repeated in many places (such as telling people they need to update their collar). If we could make these scripts leaner by using Firestorm includes, that would be its own kind of accessibility improvement. At the same time, it would be a very big improvement in maintainability to be able to change such things in one place instead of many.

Thoughts on this? Downsides I'm missing?

TakatSu1 commented 6 years ago

I'm not sure that you'll see the benefits that you're hoping for. First of all, it means that each developer is going to have to have the include files on their personal computer, and set up for Firestorm include. Secondly, not everyone is using firestorm. Thirdly, someone perusing the code who does not have firestorm preprocessor enabled will see a much more difficult to understand script.

On the other hand, maintaining a single set of constants is a stability and QA advantage. I don't know how many times I've updated a constant in one file and forgot to update it in some other, little accessed, function so that some point down the road stuff starts to break (not just in LSL).

If you're imagining only a small-ish group of people ever looking at the scripts, then sure, go with it. Maintaining a development environment is probably doable. If you're hoping for many others to even casually tinker with and contribute, that might be asking a bit much of them.

silentwar-resident commented 6 years ago

Please explain more, what does it have to do with firestorm and how does collar work involving firestorm viewer?

From: nirea Sent: Sunday, November 26, 2017 10:06 PM To: OpenCollarTeam/OpenCollar Cc: Subscribed Subject: [OpenCollarTeam/OpenCollar] Switch to Firestorm preprocessor? (#39)

This is an issue that I've gone back and forth on. I used to be strongly opposed to using the Firestorm preprocessor because I wanted people to be able to just open up their collars and see the same code that they'd see on Github. Accessibility of the code for new users is important to me. On the other hand, in going through the code tonight, I'm seeing TONS of duplication. I already know that most scripts have the same constants at the top, but there are also text strings repeated in many places (such as telling people they need to update their collar). If we could make these scripts leaner by using Firestorm includes, that would be its own kind of accessibility improvement. At the same time, it would be a very big in in maintainability to be able to change such things in one place instead of many. Thoughts on this? Downsides I'm missing?

pixelwork commented 6 years ago

i dont like firestorm, i know its a very popular viewer this days but i used cool viewer right from start, when it was not officially 64bit yet i used to build my own 64bit version of it and maintain it. even this days i compile cool viewer from sources for myself with a few patches.

Marines viewer is also quite popular in the scene since she has new RLV features first RLVa in firestorm is often months if not years behind

so i also vote against making things too specific for a viewer.

TakatSu1 commented 6 years ago

Silent, Firestorm has the option of invoking the Firestorm Preprocessor. It has the ability, amongst other things, to include files, much like C's "#include" or PHP's "require (..)" directives/calls. It also has other abilities too, such as switch statements. However, obviously using any of these functions then requires that all developers develop using the firestorm viewer, which could be problematic for a number of reasons.

nirea commented 6 years ago

It sounds like Firestorm would be a real blocker to some people who want to be involved in the code. Let's not make this change. Thank you everybody for your input!