Representative, stable, updated viewer |ignr.0x22

MeertenR commented 4 years ago

Bandaid and upgrade commit

If applied, this commit will offer a stable and representative viewer while we still do work on supporting core 0x22 simulation which currently feeds malformed data and destabilizes the viewer.

Resolves issues #4, #11, #1097, #1283 Tested positively by a handful of people.

Featuring

Intelligent core selection

Malformed data from 0x22 cores is currently known to misrepresent the protein and crash the application to desktop.

Until this is fixed, this 'bandaid' edition will first query the SimulationInfo to check for coreType. Then only if the core proves compatible will the viewer request to load up on its trajectory data. This procedure will take the protein a couple of seconds longer to load.

On startup, the viewer will select the slot that has or may have a compatible slot running. Should only an incompatible core be available then it will only show its progress-info while clearly stating "INCOMPAT" at the 'Protein:' status field.

The latter also applies when manually switched to by the user.

Result: a real protein and application stability.

Extended status information

Introducing another new status message: "Awaiting" before "Loading".

This label is used when the client has connected but is still waiting for a compatible slot with WU to commence loading. After such a slot is presented it will change to "Loading" while trajectory data is actually being loaded in.

Result: improved communication.

Wiggling

Now on/off toggleable with 'w'. It's still enabled by default, as it should be.

Result: easier to see the actual folding movements.

Rotation

Now on/off toggleable with 'r' in addition to the arrow control keys.

Default speed adjusted so that it doesn't distract too much from the folding movements themselves. Relevant for bigger, non-demo proteins.

Result: easier to see the actual folding movements.

Turbo rendering mode

Toggleable with 't'.

This will push fps to 40-75 depending on protein complexity and CPU.

While more or less preserving a normal simulation speed, as InterpSteps are dynamically increased accordingly.

Result: better demonstrations.

Increase base framerate to 16

This is for all the users who will never find the "Help" button. (>30%?)

It is my opinion that given the manual and therefore conscious nature of opening FAHViewer, users should enjoy a somewhat more decent fps by default.

The automatic screensaver, on the contrary, can still be left at its leaner settings as it may be less of a conscious choice.

Result: more modern default user experience.

Change default resolution back to 1024x768 on Windows

The only reason we're still at 800x600 is because of #1081.

We can work around this issue by using GetDesktopWindow to return available screen size without taskbar. If those values are over 1024x768 then we're good to init with that.

Result: more modern default user experience.

Pop-up windows

Can now be closed with the same key. To-do: making the F@H logo clickable leading to the home page/forum.

Result: intuitiveness.

Discussion @ https://foldingforum.org/viewtopic.php?f=106&t=32628 Feel free to modify to your liking.

jcoffland commented 4 years ago

This looks really good. The only thing I ask is to please maintain the coding style. Here are some things I see.

Two space indent
Curly brackets on same line as if, else, etc.
No curly brackets for single statements, unless it's an if within an if.
single space around all operators, including assignment.
Two lines between functions.
Strictly enforced 80 column width.

This piece of code is a good example:

 if (trajectory->empty() && !client.isNull())
  {
    if (info.coreType == 34)
      return "INCOMPAT";
    else if (client->hasLoadableSlot())
      return "Loading";
    else
      if (client->isConnected())
        return "Awaiting";
      else
        return "";
  }
  else
  return info.project ? "Live" : "Demo";

I would format it this way:

 if (trajectory->empty() && !client.isNull()) {
    if (info.coreType == 34) return "INCOMPAT";
    else if (client->hasLoadableSlot()) return "Loading";
    else if (client->isConnected()) return "Awaiting";
    else return "";

  } else return info.project ? "Live" : "Demo";

I put emphasis on not wasting vertical space so more code is visible on one screen.

MeertenR commented 4 years ago

Thank you!

Good thing I hadn't marked anything for review yet. Reason being that someone asked me to push what I got so far, so that they could follow along.

Thank you for this valuable feedback tho. Perhaps I can pick up a few things from your personal coding style. As such you have my permission to nitpick as if I'm coding for the 777X.

Within reason, ofcourse. e.g. I can't find lines in your code like this one in View.cpp:

  options.addTarget("password", password, "A password for accessing the remote "
                    "client")->setObscured();

Which suggests to me that your 80-char rule is not that strict. And that therefore my handful of 81-84 lines would've been perfectly acceptable.

Also sometimes I'll be between two fires e.g.:

          if (list.getDict(i)["description"]->getString().substr(0,3) == "cpu") {   82 characters

-VS-

          if (list.getDict(i)["description"]->getString().substr(0,3) == "cpu")     80 characters
          {

At which point I may opt for the former as it would seem the lesser of evils.

Anyhow - I can certainly see what you mean with your example. While I personally prefer to scroll more and compact less, I can see how others may prefer differently and that contributors should adjust to the dominant coding style. This will be changed.

As to your criticism "Two lines between functions.": What you've seen here is a single occasion where I deliberately put one newline between the incFPS() and decFPS() functions, because these go together.

Here I am making use of the Gestalt law of proximity to signal the level of intimacy to the brain. Contrasted with the rest of the code, the lack of a second newline provides a stimulus to group.

While I'd suggest you to put this principle to good use, for now I will add the line break as requested.

I'll be pushing a new commit that should include the changes that we've discussed here. Please do let me know should you find any more technical debt.

Meerten

MeertenR commented 4 years ago

Joseph?

jcoffland commented 3 years ago

Sorry, just seeing this now.

  options.addTarget("password", password, "A password for accessing the remote "
                    "client")->setObscured();
Which suggests to me that your 80-char rule is not that strict.

By my measure the above line is exactly 80-char wide.

          if (list.getDict(i)["description"]->getString().substr(0,3) == "cpu") { 82 characters

-VS-

          if (list.getDict(i)["description"]->getString().substr(0,3) == "cpu")       80 characters
          {

At which point I may opt for the former as it would seem the lesser of evils.

I would break it like this:

          if (list.getDict(i)["description"]->getString().substr(0,3) ==
            "cpu") {

or even better:

          string type = list.getDict(i)["description"]->getString().substr(0,3);
          if (type == "cpu") {

FoldingCommunity / fah-viewer

Representative, stable, updated viewer |ignr.0x22 #21

Featuring