Partial recorder bug fix + app version included in logs / titlebar #102

NickIs211 · 2022-09-15T00:31:36Z

No description provided.

src/main/obsRecorder.ts

tdaugaard · 2022-09-15T18:42:55Z

I think there might be a better way to fix the issue of this +/- 1 pixel thing due to the scaling; someone said somewhere on the Internet that you could simply use Math.floor() on the calculation where it multiplies by the scaling factor. Perhaps that would be better to test out?

* feb22 backup * fix playerCombatant bug

* Cleanup application status values and logic - Create an enum for the application status values in a new module, `main/types`. - Replace `if..else` with an object of messages mapped to the new `AppStatus` enums. * Address comments in PR aza547#104

aza547 · 2022-09-15T20:35:07Z

I think there might be a better way to fix the issue of this +/- 1 pixel thing due to the scaling; someone said somewhere on the Internet that you could simply use Math.floor() on the calculation where it multiplies by the scaling factor. Perhaps that would be better to test out?

I think this is what @NickIs211 initially suggested in discord and I steered away from it. Might have been right all along. Not had a chance to look over this beyond the comments yet so not sure of my opinion. Will get a chance at the weekend.

Add `enum UnitFlag` to types for easier identification of the various flags that a unit can have. Add `isUnitPlayer()` to easier identify a player controlled unit. Refactor `isUnitSelf()` to make use of `isUnitPlayer()` and `UnitFlags`.

* Remember volume and mute state when changing video * Avoid re-render when not needed We don't need to use `useState()` for reading and storing the current settings of the video player, only during the `useEffect()` call when `state.videoIndex` changes as the current state is being passed to `useState()` when the video changes in `handleChangeVideo()`.

aza547

Hi, thanks for the PR, sorry for taking so long to get to it (and I've still only had a second glance). There is a bunch of changes here that I think are due to other PRs. Could you catch up with main by either merging it in or rebasing please?

I've quickly checked the version stuff which is awesome, curious why I see 18.0.3 on dev builds? You mentioned in comments that it works fine for release builds. Why the difference? I searched the repo for 18.0.3 and couldn't find a reference to it! Where is it coming from?

I've not managed to review the OBS bits as I'm not awake enough, but I promise I'll get to them soon!

src/main/main.ts

…za547#106) * Fix problem with `maxSplits` in `splitLogLine()` `break` wasn't breaking out of both the `switch` and the `for` blocks, which is should have, causing us to not limit the number of splits we did but also to split incorrectly as `value` wasn't cleared before starting the next element value. * Fix off-by-one for month in `LogLine`date()` `Date.getMonth()` returns a zero based month number, with January being 0 and December being 11. This caused months to be off-by-one during parsing into a proper date.

* Refactor `categories` to be an enum type * Address comments in PR aza547#109

* Initial attempt at adding min encounter duration * Add default minimum encounter duration setting * Clean up syntax / formatting

aza547

Hi, happy enough with the function, but a few code style comments for you to fix up. Thanks!

src/main/obsRecorder.ts

aza547 · 2022-09-18T10:18:27Z

src/main/obsRecorder.ts

+    return isClose(parseInt(resW, 10), physicalWidth) && isClose(parseInt(resH, 10), physicalHeight);
+  };
+
+  // TODO: Output should eventually be moved into a setting field to be scaled down. For now it matches the monitor resolution.


Where did this information come from? It's news to me - I'm quite keen to understand OBS better so if you have knowledge please share!

Video.Base translates to Base Canvas Resolution, which is the size of the scene we are creating. For current purposes, this should always match monitor resolution.
Video.Output translates to Output Resolution, which is the video resolution we're recording at. For users where storage space is a concern, they should be able to change the output resolution to a size that they find acceptable, say 720p. Currently, with this setup, users with 4k monitors will record at 4k.

Ah I see. That's interesting for #67 if you fancy tackling another issue after!

src/main/obsRecorder.ts

NickIs211 · 2022-09-18T11:35:06Z

@aza547 code refactored based on comments above

aza547

Nice. This looks good, with the only problem being I'm not sure what happens if I try to merge this given the massive "files changed" page. I'm not good enough with git to know how to resolve (thought merge / rebase would do it) - can you either fix that via some git means (if you are better with it than me!) or re-create the PR so it's got a clean diff? Then I'm happy to merge!

aza547 · 2022-09-18T11:51:47Z

src/main/obsRecorder.ts

+    return isClose(parseInt(resW, 10), physicalWidth) && isClose(parseInt(resH, 10), physicalHeight);
+  };
+
+  // TODO: Output should eventually be moved into a setting field to be scaled down. For now it matches the monitor resolution.


Ah I see. That's interesting for #67 if you fancy tackling another issue after!

aza547 · 2022-09-18T11:55:07Z

src/main/obsRecorder.ts

  // Update source settings:
  let settings = videoSource.settings;
  settings['monitor'] = monitorIndexFromZero;
-  settings['width'] = physicalWidth;


Were these doing anything at all?

They weren't being respected when creating anything really, and looking at

https://github.com/stream-labs/obs-studio-node/blob/5e22aaa87103ddc4f234d6b0f73d173e9b76b507/tests/osn-tests/util/input_settings.ts#L106
https://github.com/stream-labs/obs-studio-node/blob/d048994aac4c3988e5f300d835feabdf8e57a3df/tests/osn-tests/src/test_osn_input.ts#L134

They don't seem to be used properties during osn testing. Also couldn't find the related fields in the libobs docs.

aza547 · 2022-09-18T11:58:41Z

Also in the new PR, please add a changelog fixed entry linked to #96.

NickIs211 · 2022-09-18T13:04:49Z

Nice. This looks good, with the only problem being I'm not sure what happens if I try to merge this given the massive "files changed" page. I'm not good enough with git to know how to resolve (thought merge / rebase would do it) - can you either fix that via some git means (if you are better with it than me!) or re-create the PR so it's got a clean diff? Then I'm happy to merge!

Should be all good now. I initially merged main branch instead of bug-fixes ;;;;;;
@aza547

aza547 · 2022-09-18T13:49:00Z

Nice. This looks good, with the only problem being I'm not sure what happens if I try to merge this given the massive "files changed" page. I'm not good enough with git to know how to resolve (thought merge / rebase would do it) - can you either fix that via some git means (if you are better with it than me!) or re-create the PR so it's got a clean diff? Then I'm happy to merge!

Should be all good now. I initially merged main branch instead of bug-fixes ;;;;;; @aza547

Hmm I still see 22 files changed (and lots of stuff from other commits). I think I was expecting merging main to be the correct thing to do (I'm not sure what bug-fixes is/was).

…za547#96

tdaugaard reviewed Sep 15, 2022

View reviewed changes

src/main/obsRecorder.ts Outdated Show resolved Hide resolved

tdaugaard reviewed Sep 15, 2022

View reviewed changes

src/main/obsRecorder.ts Outdated Show resolved Hide resolved

aza547 and others added 2 commits September 15, 2022 20:07

Few bug fixes (aza547#101)

46721a7

* feb22 backup * fix playerCombatant bug

tdaugaard and others added 5 commits September 16, 2022 22:01

Fix wrong/missing import for AppStatus (aza547#110)

11ea2ff

fix icon size, add log format stuff

4a72500

Cleanup the isUnitSelf() method (aza547#108)

e2bf656

Add `enum UnitFlag` to types for easier identification of the various flags that a unit can have. Add `isUnitPlayer()` to easier identify a player controlled unit. Refactor `isUnitSelf()` to make use of `isUnitPlayer()` and `UnitFlags`.

cl

e9608f1

aza547 reviewed Sep 16, 2022

View reviewed changes

src/main/main.ts Show resolved Hide resolved

tdaugaard and others added 7 commits September 16, 2022 23:41

Refactor 'categories' to be an enum type (aza547#109)

3957fe4

* Refactor `categories` to be an enum type * Address comments in PR aza547#109

log stuff

b5f0096

Add minimum encounter duration setting (aza547#107)

9314144

* Initial attempt at adding min encounter duration * Add default minimum encounter duration setting * Clean up syntax / formatting

raid reset tests & changelog

9a9d60c

contr

a5a6622

cl

98a98db

aza547 requested changes Sep 18, 2022

View reviewed changes

aza547 mentioned this pull request Sep 18, 2022

Add a version check #74

Closed

NickIs211 added 4 commits September 18, 2022 09:31

Fixed partial recording on displays with non-whole scale numbers

4c7e74a

added version to title bar

161bdc6

version bump

64a24c4

Added comment about getVersion during development. PR Also addresses a…

d2902dd

…za547#96

NickIs211 added 3 commits September 18, 2022 09:32

Rewrote checkRes code based on github review

cf62115

changed obsRecorder.setupScene to use getAvailableValues and setSetting

269286a

refactored resolution finding for scene

93af6e0

NickIs211 force-pushed the partial-recorder-bug branch from 965cf88 to 93af6e0 Compare September 18, 2022 14:36

NickIs211 closed this Sep 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Partial recorder bug fix + app version included in logs / titlebar #102

Partial recorder bug fix + app version included in logs / titlebar #102

NickIs211 commented Sep 15, 2022

tdaugaard commented Sep 15, 2022

aza547 commented Sep 15, 2022

aza547 left a comment

aza547 left a comment

aza547 Sep 18, 2022

NickIs211 Sep 18, 2022

aza547 Sep 18, 2022

NickIs211 commented Sep 18, 2022

aza547 left a comment

aza547 Sep 18, 2022

aza547 Sep 18, 2022

NickIs211 Sep 18, 2022

aza547 commented Sep 18, 2022

NickIs211 commented Sep 18, 2022 •

edited

Loading

aza547 commented Sep 18, 2022

Partial recorder bug fix + app version included in logs / titlebar #102

Partial recorder bug fix + app version included in logs / titlebar #102

Conversation

NickIs211 commented Sep 15, 2022

tdaugaard commented Sep 15, 2022

aza547 commented Sep 15, 2022

aza547 left a comment

Choose a reason for hiding this comment

aza547 left a comment

Choose a reason for hiding this comment

aza547 Sep 18, 2022

Choose a reason for hiding this comment

NickIs211 Sep 18, 2022

Choose a reason for hiding this comment

aza547 Sep 18, 2022

Choose a reason for hiding this comment

NickIs211 commented Sep 18, 2022

aza547 left a comment

Choose a reason for hiding this comment

aza547 Sep 18, 2022

Choose a reason for hiding this comment

aza547 Sep 18, 2022

Choose a reason for hiding this comment

NickIs211 Sep 18, 2022

Choose a reason for hiding this comment

aza547 commented Sep 18, 2022

NickIs211 commented Sep 18, 2022 • edited Loading

aza547 commented Sep 18, 2022

NickIs211 commented Sep 18, 2022 •

edited

Loading