Skip to content
This repository has been archived by the owner on Jul 18, 2024. It is now read-only.

Commit

Permalink
Prevent unnecessary re-renders in 3D panel when selecting source (#7413)
Browse files Browse the repository at this point in the history
**User-Facing Changes**
<!-- will be used as a changelog entry -->
- Prevent unnecessary re-renders in 3D panel after selecting a source

**Description**
The ThreeDeePanel would render 3 times when a source was selected when
it only needs to rerender once. This is because after the player is set
for the source, a new messagepipleine store is created and the player
along with its source need to get initialized. To limit rerenders in
Extension panels while initializing a new player I've added some logic
to not call `initPanel` when the player `presence` is `INITIALIZING`. I
also had to update `defaultPlayerState` in the messagepipeline store to
take an optional parameter of whether it was initialized with the player
so that it can initialize the default player state to `INITIALIZING`
when there is a player present for the store or `NOT_PRESENT` when there
is not a player (on startup).

A drawback of this solution is that it only applies to extension panels,
but might be useful for other panels as well. One other consideration
might be to prevent rendering the mosaic layout while the player is
initializing to prevent thrashing and show a loading indicator instead.
I wasn't sure what the right place to leverage this would be or if it
would be desirable.


<!-- link relevant GitHub issues -->
FG-6410
<!-- add `docs` label if this PR requires documentation updates -->
<!-- add relevant metric tracking for experimental / new features -->
  • Loading branch information
snosenzo authored Feb 12, 2024
1 parent 22929f4 commit ed3946c
Show file tree
Hide file tree
Showing 3 changed files with 26 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ describe("MessagePipelineProvider/useMessagePipeline", () => {
playerState: {
activeData: undefined,
capabilities: [],
presence: PlayerPresence.NOT_PRESENT,
presence: PlayerPresence.INITIALIZING,
playerId: "",
progress: {},
},
Expand Down Expand Up @@ -199,7 +199,7 @@ describe("MessagePipelineProvider/useMessagePipeline", () => {
expect(all[0]!.playerState).toEqual({
activeData: undefined,
capabilities: [],
presence: PlayerPresence.NOT_PRESENT,
presence: PlayerPresence.INITIALIZING,
playerId: "",
progress: {},
});
Expand Down Expand Up @@ -259,7 +259,7 @@ describe("MessagePipelineProvider/useMessagePipeline", () => {
playerState: {
activeData: undefined,
capabilities: [],
presence: PlayerPresence.NOT_PRESENT,
presence: PlayerPresence.INITIALIZING,
playerId: "",
progress: {},
},
Expand Down
8 changes: 5 additions & 3 deletions packages/studio-base/src/components/MessagePipeline/store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,11 @@ import isDesktopApp from "@foxglove/studio-base/util/isDesktopApp";
import { FramePromise } from "./pauseFrameForPromise";
import { MessagePipelineContext } from "./types";

export function defaultPlayerState(): PlayerState {
export function defaultPlayerState(player?: Player): PlayerState {
return {
presence: PlayerPresence.NOT_PRESENT,
// when there is a player we default to initializing, to prevent thrashing in the UI when
// the player is initialized.
presence: player ? PlayerPresence.INITIALIZING : PlayerPresence.NOT_PRESENT,
progress: {},
capabilities: [],
profile: undefined,
Expand Down Expand Up @@ -138,7 +140,7 @@ export function createMessagePipelineStore({
},

public: {
playerState: defaultPlayerState(),
playerState: defaultPlayerState(initialPlayer),
messageEventsBySubscriberId: new Map(),
subscriptions: [],
sortedTopics: [],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ import useGlobalVariables from "@foxglove/studio-base/hooks/useGlobalVariables";
import {
AdvertiseOptions,
PlayerCapabilities,
PlayerPresence,
SubscribePayload,
} from "@foxglove/studio-base/players/types";
import {
Expand Down Expand Up @@ -118,7 +119,7 @@ function PanelExtensionAdapter(
const { playerState, pauseFrame, setSubscriptions, seekPlayback, sortedTopics } =
messagePipelineContext;

const { capabilities, profile: dataSourceProfile } = playerState;
const { capabilities, profile: dataSourceProfile, presence: playerPresence } = playerState;

const { openSiblingPanel, setMessagePathDropConfig } = usePanelContext();

Expand Down Expand Up @@ -547,6 +548,8 @@ function PanelExtensionAdapter(
);
}, [initialState, highestSupportedConfigVersion]);

const playerIsInitializing = playerPresence === PlayerPresence.INITIALIZING;

// Manage extension lifecycle by calling initPanel() when the panel context changes.
//
// If we useEffect here instead of useLayoutEffect, the prevRenderState can get polluted with data
Expand All @@ -558,7 +561,12 @@ function PanelExtensionAdapter(

// If the config is too new for this panel to support we bail and don't do any panel initialization
// We will instead show a warning message to the user
if (configTooNew) {
// Also don't show panel when the player is initializing. The initializing state is temporary for
// players to go through to load their sources. Once a player has completed initialization `initPanel` is called again (or even a few times),
// because parts of the player context have changed. This cleans up the old panel that was present
// during initialization. So there can be no state held between extension panels between initialization and
// whatever follows it. To prevent this unnecessary render, we do not render the panel during initialization.
if (configTooNew || playerIsInitializing) {
return;
}

Expand Down Expand Up @@ -596,7 +604,14 @@ function PanelExtensionAdapter(
getMessagePipelineContext().setSubscriptions(panelId, []);
getMessagePipelineContext().setPublishers(panelId, []);
};
}, [initPanel, panelId, partialExtensionContext, getMessagePipelineContext, configTooNew]);
}, [
initPanel,
panelId,
partialExtensionContext,
getMessagePipelineContext,
configTooNew,
playerIsInitializing,
]);

const style: CSSProperties = {};
if (slowRender) {
Expand Down

0 comments on commit ed3946c

Please sign in to comment.