Skip to content

Deprecate pc.makeArray#2289

Merged
willeastcott merged 1 commit intomasterfrom
remove-makearray
Jul 15, 2020
Merged

Deprecate pc.makeArray#2289
willeastcott merged 1 commit intomasterfrom
remove-makearray

Conversation

@willeastcott
Copy link
Copy Markdown
Contributor

pc.makeArray is private API. But it's pretty useless considering its behavior can be achieved with Array.prototype.slice.call. It's been officially moved to deprecated.js.

I confirm I have signed the Contributor License Agreement.

@willeastcott willeastcott requested a review from a team July 15, 2020 12:51
@willeastcott willeastcott self-assigned this Jul 15, 2020
Copy link
Copy Markdown
Member

@slimbuck slimbuck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering whether we could place timings on deprecated items saying they will be removed at such-and-such a date? Currently everyone pays the price of deprecated code.

@willeastcott willeastcott merged commit f8f55db into master Jul 15, 2020
@willeastcott willeastcott deleted the remove-makearray branch July 15, 2020 14:01
@willeastcott
Copy link
Copy Markdown
Contributor Author

That's one option. But it may be that it's unnecessary to ever remove deprecated stuff and instead just provide a legacy build and a non-legacy build. That's now easy with rollup - we just add another target minus the deprecated.js module. We could even make that a publish option in the Editor.

TheJonRobinson added a commit to Mojiworks/playcanvas-engine that referenced this pull request Jul 15, 2020
* mojiworks/pr/sound-stop-typing: (112 commits)
  fix: SoundComponent.stop's string parameter is optional
  [RELEASE] v1.32.0
  [Fix] Changes to Attribute to location mapping (playcanvas#2292)
  Deprecate pc.makeArray (playcanvas#2289)
  [FIX] API ref docs for pc.Mesh#getIndices (playcanvas#2291)
  fix cubemap filtering on safari (playcanvas#2288)
  disable ImageBitmap (playcanvas#2286)
  [Fix] Fix to texture based skinning on Safari (playcanvas#2287)
  fix cubemap dependent loads (playcanvas#2285)
  [Fix] Update to Transform Feedback to work with VAOs (playcanvas#2284)
  Update the anim state graph to support the new data schema (playcanvas#2278)
  Remove viewer (playcanvas#2282)
  [FIX] pc.SoundComponent#resume/stop param type (playcanvas#2281)
  Remove type function (playcanvas#2273)
  Remove some shader-related circular dependencies (playcanvas#2274)
  Glb-parser improvements (playcanvas#2275)
  Fix glb-parser normals generation (playcanvas#2276)
  Glb texture coordinate fix (playcanvas#2252)
  use faces if the asset was previously unloaded (playcanvas#2272)
  fix material resource loader (playcanvas#2268)
  ...

# Conflicts:
#	src/scene/graph-node.js
@Maksims
Copy link
Copy Markdown
Collaborator

Maksims commented Jul 17, 2020

Taking in account some deprecated stuff is still within code, for example WebVR - is deprecated, but still there.
One of the way to go, is to add another define, called LEGACY, we can then wrap deprecated stuff into LEGACY define, and that clean new engine can be compiled.

One argument would be "but we add another define", yep. But we probably could combine PROFILE and DEBUG into same target.

But probably, legacy should be still a default engine for most people, to avoid unnecessary confusion "why it suddenly breaks". And provide Launch and Publish options, to run without LEGACY stuff.

@willeastcott
Copy link
Copy Markdown
Contributor Author

Yeah, that's certainly one option. We do need to deal with:

  • WebVR
  • AudioSource component
  • Legacy Script component
  • Zone component

@Maksims
Copy link
Copy Markdown
Collaborator

Maksims commented Jul 17, 2020

Sad to see Zone component to go away, it definitely worth re-thinking it, as often wanted to separate spaces in zones, and do some clustering, and other logic based on them (fill zone with probes e.g.).

@willeastcott
Copy link
Copy Markdown
Contributor Author

It's not guaranteed we'll get rid of the zone component. But right now, there's little to no benefit from it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants